On Sat, 22 Feb 2025, Steve French wrote: > I didn't see the cifs part of this series either. Did I miss it? > > On Fri, Feb 21, 2025, 10:56 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > On Fri, Feb 21, 2025 at 10:36:30AM +1100, NeilBrown wrote: > > > Not all filesystems reliably result in a positive hashed dentry: > > > > - NFS, cifs, hostfs will sometimes need to perform a lookup of > > the name to get inode information. Races could result in this > > returning something different. Note that this lookup is > > non-atomic which is what we are trying to avoid. Placing the > > lookup in filesystem code means it only happens when the > filesystem > > has no other option. > > At least in case of cifs I don't see that lookup anywhere in your > series. Have I missed it, or...? I didn't make any interesting changes to cifs. I wasn't sure that I needed to... cifs already does the lookup - sometimes. cifs_get_unix_fattr() (I think) does a lookup based on the full path name. Though now I see that is only enabled with CONFIG_CIFS_ALLOW_INSECURE_LEGACY. This is sometimes called by cifs_mkdir_qinfo() which is called after success in cifs_mkdir(). This creates the inode and called d_instantiate(). As that isn't d_instantiate_new() it maybe needs to be d_splice_alias(). However cifs doesn't provide open_by_handle_at() functionality so one race that can sometimes cause a problem cannot happen. Also of the three clients of vfs_mkdir which care about the dentry, - cachefiles will still repeat the lookup - nfsd cannot export cifs, so its behaviour isn't relevant - smb/server - even if it can re-export cifs the only down-side is that the uid won't be written by ksmbd_vfs_inherit_owner() but I wonder of persistent of i_uid_write() without a notify_change() could be on a network filesystem... I should probably change the d_instantiate() to d_drop();d_splice_alias(). I'm not able to review all the paths to determine if a lookup might be needed somewhere or to add any lookup. Should d_instantiate() WARN_ON when the inode->i_dentry list isn't empty for a directory?? Thanks, NeilBrown
