On 21/12/21 03:13PM, Chuck Lever III wrote: > > > > On Dec 21, 2021, at 6:31 AM, Pratyush Yadav <p.yadav@xxxxxx> wrote: > > > > Hi, > > > > On 17/12/21 04:11PM, Chuck Lever III wrote: > >> > >>> On Dec 17, 2021, at 1:41 AM, Vasily Averin <vvs@xxxxxxxxxxxxx> wrote: > >>> > >>> On 16.12.2021 20:20, J. Bruce Fields wrote: > >>>> From: "J. Bruce Fields" <bfields@xxxxxxxxxx> > >>>> > >>>> 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@xxxxxxxxxxxxx> > >>>> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx> > >>> > >>> Reviewed-by: Vasily Averin <vvs@xxxxxxxxxxxxx> > >> > >> 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?). > > I updated this patch in my for-next tree yesterday to take care of > the issue. The change has been merged into today's linux-next. Thanks. I updated to today's linux-next and I no longer see this error. The build still fails though, this time on some firmware driver. I'll go bother their maintainers about it now ;-) -- Regards, Pratyush Yadav Texas Instruments Inc.
