Date: Sun, 22 Jan 2017 12:07:56 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Mateusz Guzik <mjg@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312600 - head/sys/kern Message-ID: <20170122115716.T953@besplex.bde.org> In-Reply-To: <20170121195114.GA2349@kib.kiev.ua> References: <201701211838.v0LIcHIv072626@repo.freebsd.org> <20170121195114.GA2349@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 21 Jan 2017, Konstantin Belousov wrote:
> On Sat, Jan 21, 2017 at 06:38:17PM +0000, Mateusz Guzik wrote:
>> ...
>> Log:
>> vfs: refactor _vn_lock
>>
>> Stop testing for LK_RETRY and error multiple times. Also postpone the
>> VI_DOOMED until after LK_RETRY was seen as it reads from the vnode.
>>
>> No functional changes.
Many style bugs.
>> Modified: head/sys/kern/vfs_vnops.c
>> ==============================================================================
>> --- head/sys/kern/vfs_vnops.c Sat Jan 21 17:39:10 2017 (r312599)
>> +++ head/sys/kern/vfs_vnops.c Sat Jan 21 18:38:16 2017 (r312600)
>> @@ -1539,27 +1539,24 @@ _vn_lock(struct vnode *vp, int flags, ch
>>
>> VNASSERT((flags & LK_TYPE_MASK) != 0, vp,
>> ("vn_lock called with no locktype."));
>> - do {
>> #ifdef DEBUG_VFS_LOCKS
>> - KASSERT(vp->v_holdcnt != 0,
>> - ("vn_lock %p: zero hold count", vp));
>> + KASSERT(vp->v_holdcnt != 0,
>> + ("vn_lock %p: zero hold count", vp));
> This line also has wrong inde seems to not be fixed by later commit.
>
>> #endif
>> - error = VOP_LOCK1(vp, flags, file, line);
>> - flags &= ~LK_INTERLOCK; /* Interlock is always dropped. */
>> - KASSERT((flags & LK_RETRY) == 0 || error == 0,
>> - ("LK_RETRY set with incompatible flags (0x%x) or an error occurred (%d)",
>> - flags, error));
This was correctly formatted except for the long line.
>> - /*
>> - * Callers specify LK_RETRY if they wish to get dead vnodes.
>> - * If RETRY is not set, we return ENOENT instead.
>> - */
>> - if (error == 0 && vp->v_iflag & VI_DOOMED &&
>> - (flags & LK_RETRY) == 0) {
>> +retry:
>> + error = VOP_LOCK1(vp, flags, file, line);
>> + flags &= ~LK_INTERLOCK; /* Interlock is always dropped. */
>> + KASSERT((flags & LK_RETRY) == 0 || error == 0,
>> + ("LK_RETRY set with incompatible flags (0x%x) or an error occurred (%d)",
>> + flags, error));
This is even more grossly misreformatted than the other KASSERT(). The new
style bugs are:
- random first contuation indent (it happens to be 2 tabs)
- long line longer than before (further messed up by the indentation)
- random second contuation indent (it actually lines up with the first,
except it uses gnu-style lining up parentheses instead of KNF)
>> + if (flags & LK_RETRY) {
> Stylish test is
> if ((flags & LK_RETRY) != 0) {
>> + if ((error != 0))
> Too many ().
>
>> + goto retry;
>> + if ((vp->v_iflag & VI_DOOMED)) {
> Too many braces again, or missed != 0.
>> VOP_UNLOCK(vp, 0);
>> error = ENOENT;
>> - break;
> Also, this does the functional change, it seems to completely break LK_RERY
> logic. If LK_RETRY is specified, VI_DOOMED must not result in ENOENT.
Later commits further unimproved style by adding __predict_ugly() and
long lines from blind substitution of that. __predict_ugly() is almost
as useful as 'register', but more invasive.
>
>> }
>> - } while (flags & LK_RETRY && error != 0);
>> + }
>> return (error);
>> }
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170122115716.T953>