Date:   Tue, 21 Dec 2021 17:01:39 +0530
From:   Pratyush Yadav <p.yadav@ti.com>
To:     Chuck Lever III <chuck.lever@oracle.com>
CC:     Vasily Averin <vvs@virtuozzo.com>,
        Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Jeff Layton <jlayton@kernel.org>,
        "Denis V. Lunev" <den@virtuozzo.com>,
        Cyrill Gorcunov <gorcunov@virtuozzo.com>,
        Konstantin Khorenko <khorenko@virtuozzo.com>
Subject: Re: [PATCH] nfs: block notification on fs with its own ->lock
Message-ID: <20211221113139.6to2hact5qfa2sbd@ti.com>
References: <20211216172013.GA13418@fieldses.org>
 <bb7b5a71-6ddf-5e22-e555-8ae22e5930fe@virtuozzo.com>
 <0C441CAE-06B6-4CC0-8A52-88957DCF76EF@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <0C441CAE-06B6-4CC0-8A52-88957DCF76EF@oracle.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

Hi,

On 17/12/21 04:11PM, Chuck Lever III wrote:
> 
> > On Dec 17, 2021, at 1:41 AM, Vasily Averin <vvs@virtuozzo.com> wrote:
> > 
> > On 16.12.2021 20:20, J. Bruce Fields wrote:
> >> From: "J. Bruce Fields" <bfields@redhat.com>
> >> 
> >> NFSv4.1 supports an optional lock notification feature which notifies
> >> the client when a lock comes available.  (Normally NFSv4 clients just
> >> poll for locks if necessary.)  To make that work, we need to request a
> >> blocking lock from the filesystem.
> >> 
> >> We turned that off for NFS in f657f8eef3ff "nfs: don't atempt blocking
> >> locks on nfs reexports" because it actually blocks the nfsd thread while
> >> waiting for the lock.
> >> 
> >> Thanks to Vasily Averin for pointing out that NFS isn't the only
> >> filesystem with that problem.
> >> 
> >> Any filesystem that leaves ->lock NULL will use posix_lock_file(), which
> >> does the right thing.  Simplest is just to assume that any filesystem
> >> that defines its own ->lock is not safe to request a blocking lock from.
> >> 
> >> So, this patch mostly reverts f657f8eef3ff and b840be2f00c0, and instead
> >> uses a check of ->lock (Vasily's suggestion) to decide whether to
> >> support blocking lock notifications on a given filesystem.  Also add a
> >> little documentation.
> >> 
> >> Perhaps someday we could add back an export flag later to allow
> >> filesystems with "good" ->lock methods to support blocking lock
> >> notifications.
> >> 
> >> Reported-by: Vasily Averin <vvs@virtuozzo.com>
> >> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > Reviewed-by: Vasily  Averin <vvs@virtuozzo.com>
> 
> I've applied this with Vasily's R-b to for-next at
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
> I also cleaned up some checkpatch nits in the patch description.
> 
> It might be good for subsequent work in this area to be based
> on the for-next branch so we can track what is done and what
> is left to do.

This patch breaks LLVM build on linux-next for me:

fs/lockd/svclock.c:474:17: error: unused variable 'inode' [-Werror,-Wunused-variable]
        struct inode            *inode = nlmsvc_file_inode(file);

This is because now the only user of inode is the dprintk() call, and 
this is probably a noop when debug is disabled. I think you should wrap 
the declaration of inode under the same debug symbol used to select 
dprintk(). My LSP (ccls) is getting confused and can't point out where 
exactly this macro is declared, so I am not sure which symbol controls 
it (CONFIG_DEBUG? CONFIG_NFS_DEBUG?).

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.