Following is a new version of this series: - fixed a bug found by syzbot - cleanup suggested by Stephen Smalley - added patch for missing updates in smb/server - thanks Jeff Layton - various s-o-b Previous description: this series is the next part of my effort to change directory-op locking to allow multiple concurrent ops in a directory. Ultimately we will (in my plan) lock the target dentry(s) rather than the whole parent directory. To help with changing the locking protocol, this series centralises locking and lookup in some helpers. The various helpers are introduced and then used in the same patch - roughly one patch per helper though with various exceptions. I haven't introduced these helpers into the various filesystems that Al's tree-in-dcache series is changing. That series introduces and uses similar helpers tuned to the specific needs of that set of filesystems. Ultimately all the helpers will use the same backends which can then be adjusted when it is time to change the locking protocol. One change that deserves highlighting is in patch 13 where vfs_mkdir() is changed to unlock the parent on failure, as well as the current behaviour of dput()ing the dentry on failure. Once this change is in place, the final step of both create and an remove sequences only requires the target dentry, not the parent. So e.g. end_creating() is only given the dentry (which may be IS_ERR() after vfs_mkdir()). This helps establish the pattern that it is the dentry that is being locked and unlocked (the lock is currently held on dentry->d_parent->d_inode, but that can change). Please review the changes I've made to your respective code areas and let us know of any problems. Thanks, NeilBrown [PATCH v6 01/15] debugfs: rename end_creating() to [PATCH v6 02/15] VFS: introduce start_dirop() and end_dirop() [PATCH v6 03/15] VFS: tidy up do_unlinkat() [PATCH v6 04/15] VFS/nfsd/cachefiles/ovl: add start_creating() and [PATCH v6 05/15] VFS/nfsd/cachefiles/ovl: introduce start_removing() [PATCH v6 06/15] VFS: introduce start_creating_noperm() and [PATCH v6 07/15] smb/server: use end_removing_noperm for for target [PATCH v6 08/15] VFS: introduce start_removing_dentry() [PATCH v6 09/15] VFS: add start_creating_killable() and [PATCH v6 10/15] VFS/nfsd/ovl: introduce start_renaming() and [PATCH v6 11/15] VFS/ovl/smb: introduce start_renaming_dentry() [PATCH v6 12/15] Add start_renaming_two_dentries() [PATCH v6 13/15] ecryptfs: use new start_creating/start_removing APIs [PATCH v6 14/15] VFS: change vfs_mkdir() to unlock on failure. [PATCH v6 15/15] VFS: introduce end_creating_keep()
On Thu, 13 Nov 2025 11:18:23 +1100, NeilBrown wrote:
> Following is a new version of this series:
> - fixed a bug found by syzbot
> - cleanup suggested by Stephen Smalley
> - added patch for missing updates in smb/server - thanks Jeff Layton
> - various s-o-b
>
>
> [...]
Applied to the vfs-6.19.directory.locking branch of the vfs/vfs.git tree.
Patches in the vfs-6.19.directory.locking branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.19.directory.locking
[01/15] debugfs: rename end_creating() to debugfs_end_creating()
https://git.kernel.org/vfs/vfs/c/8b45b9a88233
[02/15] VFS: introduce start_dirop() and end_dirop()
https://git.kernel.org/vfs/vfs/c/4037d966f034
[03/15] VFS: tidy up do_unlinkat()
https://git.kernel.org/vfs/vfs/c/3661a7887462
[04/15] VFS/nfsd/cachefiles/ovl: add start_creating() and end_creating()
https://git.kernel.org/vfs/vfs/c/7ab96df840e6
[05/15] VFS/nfsd/cachefiles/ovl: introduce start_removing() and end_removing()
https://git.kernel.org/vfs/vfs/c/bd6ede8a06e8
[06/15] VFS: introduce start_creating_noperm() and start_removing_noperm()
https://git.kernel.org/vfs/vfs/c/c9ba789dad15
[07/15] smb/server: use end_removing_noperm for for target of smb2_create_link()
https://git.kernel.org/vfs/vfs/c/1ead2213dd7d
[08/15] VFS: introduce start_removing_dentry()
https://git.kernel.org/vfs/vfs/c/7bb1eb45e43c
[09/15] VFS: add start_creating_killable() and start_removing_killable()
https://git.kernel.org/vfs/vfs/c/ff7c4ea11a05
[10/15] VFS/nfsd/ovl: introduce start_renaming() and end_renaming()
https://git.kernel.org/vfs/vfs/c/5c8752729970
[11/15] VFS/ovl/smb: introduce start_renaming_dentry()
https://git.kernel.org/vfs/vfs/c/ac50950ca143
[12/15] Add start_renaming_two_dentries()
https://git.kernel.org/vfs/vfs/c/833d2b3a072f
[13/15] ecryptfs: use new start_creating/start_removing APIs
https://git.kernel.org/vfs/vfs/c/f046fbb4d81d
[14/15] VFS: change vfs_mkdir() to unlock on failure.
https://git.kernel.org/vfs/vfs/c/fe497f0759e0
[15/15] VFS: introduce end_creating_keep()
https://git.kernel.org/vfs/vfs/c/cf296b294c3b
On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > Following is a new version of this series: > - fixed a bug found by syzbot > - cleanup suggested by Stephen Smalley > - added patch for missing updates in smb/server - thanks Jeff Layton The codeflow right now is very very gnarly in a lot of places which obviously isn't your fault. But start_creating() and end_creating() would very naturally lend themselves to be CLASS() guards. Unrelated: I'm very inclined to slap a patch on top that renames start_creating()/end_creating() and start_dirop()/end_dirop() to vfs_start_creating()/vfs_end_creating() and vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level maintained helpers and I try to be consistent with the naming in the codebase making it very easy to grep.
On Fri, 14 Nov 2025, Christian Brauner wrote: > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > > Following is a new version of this series: > > - fixed a bug found by syzbot > > - cleanup suggested by Stephen Smalley > > - added patch for missing updates in smb/server - thanks Jeff Layton > > The codeflow right now is very very gnarly in a lot of places which > obviously isn't your fault. But start_creating() and end_creating() > would very naturally lend themselves to be CLASS() guards. I agree that using guards would be nice. One of my earlier versions did that but Al wants the change to use guards to be separate from other changes. I'll suggest something at some stage if no-one else does it first. > > Unrelated: I'm very inclined to slap a patch on top that renames > start_creating()/end_creating() and start_dirop()/end_dirop() to > vfs_start_creating()/vfs_end_creating() and > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level > maintained helpers and I try to be consistent with the naming in the > codebase making it very easy to grep. > I don't object to adding a vfs_ prefix. (What would be really nice is of the vfs_ code was in a separate vfs/ directory, but that is probably too intrusive to be worth it). Thanks, NeilBrown
On Fri, Nov 14, 2025 at 01:24:41PM +0100, Christian Brauner wrote: > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > > Following is a new version of this series: > > - fixed a bug found by syzbot > > - cleanup suggested by Stephen Smalley > > - added patch for missing updates in smb/server - thanks Jeff Layton > > The codeflow right now is very very gnarly in a lot of places which > obviously isn't your fault. But start_creating() and end_creating() > would very naturally lend themselves to be CLASS() guards. > > Unrelated: I'm very inclined to slap a patch on top that renames > start_creating()/end_creating() and start_dirop()/end_dirop() to > vfs_start_creating()/vfs_end_creating() and > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level > maintained helpers and I try to be consistent with the naming in the > codebase making it very easy to grep. @Neil, @Jeff, could you please look at: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.all and specifically at the merge conflict resolution I did for: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.all&id=f28c9935f78bffe6fee62f7fb9f6c5af7e30d9b2 and tell me whether it all looks sane?
On Sat, 15 Nov 2025, Christian Brauner wrote: > On Fri, Nov 14, 2025 at 01:24:41PM +0100, Christian Brauner wrote: > > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > > > Following is a new version of this series: > > > - fixed a bug found by syzbot > > > - cleanup suggested by Stephen Smalley > > > - added patch for missing updates in smb/server - thanks Jeff Layton > > > > The codeflow right now is very very gnarly in a lot of places which > > obviously isn't your fault. But start_creating() and end_creating() > > would very naturally lend themselves to be CLASS() guards. > > > > Unrelated: I'm very inclined to slap a patch on top that renames > > start_creating()/end_creating() and start_dirop()/end_dirop() to > > vfs_start_creating()/vfs_end_creating() and > > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level > > maintained helpers and I try to be consistent with the naming in the > > codebase making it very easy to grep. > > @Neil, @Jeff, could you please look at: > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.all > > and specifically at the merge conflict resolution I did for: > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.all&id=f28c9935f78bffe6fee62f7fb9f6c5af7e30d9b2 > > and tell me whether it all looks sane? > That merge is a7b062be95fed490d1dcd350d3b5657f243d7d4f today, and I agree with Jeff that it looks good. Thanks, NeilBrown
On Fri, 2025-11-14 at 15:23 +0100, Christian Brauner wrote: > On Fri, Nov 14, 2025 at 01:24:41PM +0100, Christian Brauner wrote: > > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > > > Following is a new version of this series: > > > - fixed a bug found by syzbot > > > - cleanup suggested by Stephen Smalley > > > - added patch for missing updates in smb/server - thanks Jeff Layton > > > > The codeflow right now is very very gnarly in a lot of places which > > obviously isn't your fault. But start_creating() and end_creating() > > would very naturally lend themselves to be CLASS() guards. > > > > Unrelated: I'm very inclined to slap a patch on top that renames > > start_creating()/end_creating() and start_dirop()/end_dirop() to > > vfs_start_creating()/vfs_end_creating() and > > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level > > maintained helpers and I try to be consistent with the naming in the > > codebase making it very easy to grep. > > @Neil, @Jeff, could you please look at: > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.all > > and specifically at the merge conflict resolution I did for: > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.all&id=f28c9935f78bffe6fee62f7fb9f6c5af7e30d9b2 > > and tell me whether it all looks sane? I don't see any major issues. I'm kicking off a quick pynfs test run now with it. One fairly minor nit: @@ -212,15 +210,13 @@ nfsd4_create_clid_dir(struct nfs4_client *clp) * In the 4.0 case, we should never get here; but we may * as well be forgiving and just succeed silently. */ - goto out_put; - dentry = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), dentry, 0700, NULL); + goto out_end; + dentry = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), dentry, S_IRWXU, NULL); if (IS_ERR(dentry)) status = PTR_ERR(dentry); I'm not sure if it was Neil's patch or your resolution that changed it, but the change from 0700 to a symbolic constant is not preferred, IMO. File modes are one of the few places where I think it's easier to interpret (octal) numbers rather than symbolic constants. -- Jeff Layton <jlayton@kernel.org>
On Fri, Nov 14, 2025 at 09:52:59AM -0500, Jeff Layton wrote: > On Fri, 2025-11-14 at 15:23 +0100, Christian Brauner wrote: > > On Fri, Nov 14, 2025 at 01:24:41PM +0100, Christian Brauner wrote: > > > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > > > > Following is a new version of this series: > > > > - fixed a bug found by syzbot > > > > - cleanup suggested by Stephen Smalley > > > > - added patch for missing updates in smb/server - thanks Jeff Layton > > > > > > The codeflow right now is very very gnarly in a lot of places which > > > obviously isn't your fault. But start_creating() and end_creating() > > > would very naturally lend themselves to be CLASS() guards. > > > > > > Unrelated: I'm very inclined to slap a patch on top that renames > > > start_creating()/end_creating() and start_dirop()/end_dirop() to > > > vfs_start_creating()/vfs_end_creating() and > > > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level > > > maintained helpers and I try to be consistent with the naming in the > > > codebase making it very easy to grep. > > > > @Neil, @Jeff, could you please look at: > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.all > > > > and specifically at the merge conflict resolution I did for: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.all&id=f28c9935f78bffe6fee62f7fb9f6c5af7e30d9b2 > > > > and tell me whether it all looks sane? > > > I don't see any major issues. I'm kicking off a quick pynfs test run > now with it. One fairly minor nit: > > @@ -212,15 +210,13 @@ nfsd4_create_clid_dir(struct nfs4_client *clp) > * In the 4.0 case, we should never get here; but we may > * as well be forgiving and just succeed silently. > */ > - goto out_put; > - dentry = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), dentry, 0700, NULL); > + goto out_end; > + dentry = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), dentry, S_IRWXU, NULL); > if (IS_ERR(dentry)) > status = PTR_ERR(dentry); > > I'm not sure if it was Neil's patch or your resolution that changed it, > but the change from 0700 to a symbolic constant is not preferred, IMO. > File modes are one of the few places where I think it's easier to > interpret (octal) numbers rather than symbolic constants. Neil's patches didn't change that. They just keep the status quo ante. You've changed it all to octals at the same time you extended directory operations to allow delegations. Not sure how great it is to mix those changes together in a single patch. I can change the resolution to use 0700 again ofc.
On Fri, 2025-11-14 at 13:24 +0100, Christian Brauner wrote: > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote: > > Following is a new version of this series: > > - fixed a bug found by syzbot > > - cleanup suggested by Stephen Smalley > > - added patch for missing updates in smb/server - thanks Jeff Layton > > The codeflow right now is very very gnarly in a lot of places which > obviously isn't your fault. But start_creating() and end_creating() > would very naturally lend themselves to be CLASS() guards. > > Unrelated: I'm very inclined to slap a patch on top that renames > start_creating()/end_creating() and start_dirop()/end_dirop() to > vfs_start_creating()/vfs_end_creating() and > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level > maintained helpers and I try to be consistent with the naming in the > codebase making it very easy to grep. That sounds like a good idea. The current names are a little too generic. -- Jeff Layton <jlayton@kernel.org>
© 2016 - 2025 Red Hat, Inc.