> 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. -- Chuck Lever
