On Sat 15-06-24 07:35:42, Christian Brauner wrote: > On Wed, 12 Jun 2024 17:09:55 +1000, NeilBrown wrote: > > When a file is opened and created with open(..., O_CREAT) we get > > both the CREATE and OPEN fsnotify events and would expect them in that > > order. For most filesystems we get them in that order because > > open_last_lookups() calls fsnofify_create() and then do_open() (from > > path_openat()) calls vfs_open()->do_dentry_open() which calls > > fsnotify_open(). > > > > [...] > > Applied to the vfs.fixes branch of the vfs/vfs.git tree. > Patches in the vfs.fixes 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.fixes > > [1/1] VFS: generate FS_CREATE before FS_OPEN when ->atomic_open used. > https://git.kernel.org/vfs/vfs/c/7536b2f06724 I have reviewed the patch you've committed since I wasn't quite sure which changes you're going to apply after your discussion with Amir. And I have two comments: @@ -1085,8 +1080,17 @@ EXPORT_SYMBOL(file_path); */ int vfs_open(const struct path *path, struct file *file) { + int ret; + file->f_path = *path; - return do_dentry_open(file, NULL); + ret = do_dentry_open(file, NULL); + if (!ret) + /* + * Once we return a file with FMODE_OPENED, __fput() will call + * fsnotify_close(), so we need fsnotify_open() here for symmetry. + */ + fsnotify_open(file); + return ret; } AFAICT this will have a side-effect that now fsnotify_open() will be generated even for O_PATH open. It is true that fsnotify_close() is getting generated for them already and we should strive for symmetry. Conceptually it doesn't make sense to me to generate fsnotify events for O_PATH opens/closes but maybe I miss something. Amir, any opinion here? @@ -3612,6 +3612,9 @@ static int do_open(struct nameidata *nd, int acc_mode; int error; + if (file->f_mode & FMODE_OPENED) + fsnotify_open(file); + if (!(file->f_mode & (FMODE_OPENED | FMODE_CREATED))) { error = complete_walk(nd); if (error) Frankly, this works but looks as an odd place to put this notification to. Why not just placing it just next to where fsnotify_create() is generated in open_last_lookups()? Like: if (open_flag & O_CREAT) inode_lock(dir->d_inode); else inode_lock_shared(dir->d_inode); dentry = lookup_open(nd, file, op, got_write); - if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED)) - fsnotify_create(dir->d_inode, dentry); + if (!IS_ERR(dentry)) { + if (file->f_mode & FMODE_CREATED) + fsnotify_create(dir->d_inode, dentry); + if (file->f_mode & FMODE_OPENED) + fsnotify_open(file); + } if (open_flag & O_CREAT) inode_unlock(dir->d_inode); else inode_unlock_shared(dir->d_inode); That looks like a place where it is much more obvious this is for atomic_open() handling? Now I admit I'm not really closely familiar with the atomic_open() paths so maybe I miss something and do_open() is better. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
