| Message ID | 20240131230827.207552-5-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: inode IO modes and mmap | expand |
On Thu, 1 Feb 2024 at 00:09, Bernd Schubert <bschubert@ddn.com> wrote: > > From: Amir Goldstein <amir73il@gmail.com> > > In preparation for inode io modes, a server open response could fail > due to conflicting inode io modes. > > Allow returning an error from fuse_finish_open() and handle the error in > the callers. fuse_dir_open() can now call fuse_sync_release(), so handle > the isdir case correctly. While that's true, it may be better to just decouple the dir/regular paths completely, since there isn't much sharing anyway and becoming even less. > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index d19cbf34c634..d45d4a678351 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -692,13 +692,15 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, > d_instantiate(entry, inode); > fuse_change_entry_timeout(entry, &outentry); > fuse_dir_changed(dir); > - err = finish_open(file, entry, generic_file_open); > + err = generic_file_open(inode, file); > + if (!err) { > + file->private_data = ff; > + err = finish_open(file, entry, fuse_finish_open); Need to be careful with moving fuse_finish_open() call inside finish_open() since various fields will be different. In particular O_TRUNC in f_flags will not be cleared and in this case it looks undesirable.
On Thu, Feb 1, 2024 at 11:23 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Thu, 1 Feb 2024 at 00:09, Bernd Schubert <bschubert@ddn.com> wrote: > > > > From: Amir Goldstein <amir73il@gmail.com> > > > > In preparation for inode io modes, a server open response could fail > > due to conflicting inode io modes. > > > > Allow returning an error from fuse_finish_open() and handle the error in > > the callers. fuse_dir_open() can now call fuse_sync_release(), so handle > > the isdir case correctly. > > While that's true, it may be better to just decouple the dir/regular > paths completely, since there isn't much sharing anyway and becoming > even less. > I can look into it, but for now the fix to fuse_sync_release() is a simple one liner, so I would rather limit the changes in this series. > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > > index d19cbf34c634..d45d4a678351 100644 > > --- a/fs/fuse/dir.c > > +++ b/fs/fuse/dir.c > > @@ -692,13 +692,15 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, > > d_instantiate(entry, inode); > > fuse_change_entry_timeout(entry, &outentry); > > fuse_dir_changed(dir); > > - err = finish_open(file, entry, generic_file_open); > > + err = generic_file_open(inode, file); > > + if (!err) { > > + file->private_data = ff; > > + err = finish_open(file, entry, fuse_finish_open); > > Need to be careful with moving fuse_finish_open() call inside > finish_open() since various fields will be different. > > In particular O_TRUNC in f_flags will not be cleared and in this case > it looks undesirable. Why? coming from fuse_open_common(), fuse_finish_open() is called before clearing O_TRUNC. Is fuse_finish_open() supposed to be called after clearing O_TRUNC in fuse_create_open()? I realize that this is what the code is doing in upstream, but it does not look correct. Probably, nobody could notice it, because server would probably have truncated the file before the CREATE response anyway? Am I missing something? Thanks, Amir.
On Thu, 1 Feb 2024 at 11:16, Amir Goldstein <amir73il@gmail.com> wrote: > I can look into it, but for now the fix to fuse_sync_release() is a simple > one liner, so I would rather limit the changes in this series. Not urgent, but it might be a good idea to add a cleanup patch as a prep, which would make this patch just that one line less complex. > Is fuse_finish_open() supposed to be called after clearing O_TRUNC > in fuse_create_open()? This will invalidate size/modification time, which we've just updated with the correct post open values. > I realize that this is what the code is doing in upstream, but it does not > look correct. I think it's correct, because it deals with the effect of FUSE_OPEN/O_TRUNC on attributes that weren't refreshed in contrast to fuse_create_open() where the attributes were refreshed. Thanks, Miklos
On Thu, Feb 1, 2024 at 12:29 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Thu, 1 Feb 2024 at 11:16, Amir Goldstein <amir73il@gmail.com> wrote: > > > I can look into it, but for now the fix to fuse_sync_release() is a simple > > one liner, so I would rather limit the changes in this series. > > Not urgent, but it might be a good idea to add a cleanup patch as a > prep, which would make this patch just that one line less complex. > I will see if I can get something done quickly. > > Is fuse_finish_open() supposed to be called after clearing O_TRUNC > > in fuse_create_open()? > > This will invalidate size/modification time, which we've just updated > with the correct post open values. > > > I realize that this is what the code is doing in upstream, but it does not > > look correct. > > I think it's correct, because it deals with the effect of > FUSE_OPEN/O_TRUNC on attributes that weren't refreshed in contrast to > fuse_create_open() where the attributes were refreshed. > I was considering splitting fuse_finish_open() to the first part that can fail and the "finally" part that deals with attributes, but seeing that this entire chunk of atomic_o_trunc code in fuse_finish_open() is never relevant to atomic_open(), I'd rather just move it out into fuse_open_common() which has loads of other code related to atomic_o_trunc anyway? Thanks, Amir. > Thanks, > Miklos
On Thu, 1 Feb 2024 at 11:41, Amir Goldstein <amir73il@gmail.com> wrote: > I was considering splitting fuse_finish_open() to the first part that > can fail and the "finally" part that deals with attributes, but seeing > that this entire chunk of atomic_o_trunc code in fuse_finish_open() > is never relevant to atomic_open(), I'd rather just move it out > into fuse_open_common() which has loads of other code related to > atomic_o_trunc anyway? Yep. Thanks, Miklos
On Thu, Feb 1, 2024 at 12:51 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Thu, 1 Feb 2024 at 11:41, Amir Goldstein <amir73il@gmail.com> wrote: > > > I was considering splitting fuse_finish_open() to the first part that > > can fail and the "finally" part that deals with attributes, but seeing > > that this entire chunk of atomic_o_trunc code in fuse_finish_open() > > is never relevant to atomic_open(), I'd rather just move it out > > into fuse_open_common() which has loads of other code related to > > atomic_o_trunc anyway? > > Yep. FWIW, I pushed some cleanups to: https://github.com/amir73il/linux/commits/fuse_io_mode-wip/ * e71b0c0356c8 - (github/fuse_io_mode-wip) fuse: introduce inode io modes * 081ddd63a9ff - fuse: prepare for failing open response * 437b84a47a8a - fuse: allocate ff->release_args only if release is needed * e2df18f9a3d6 - fuse: factor out helper fuse_truncate_update_attr() e2df18f9a3d6 is the O_TRUNC change discussed above. 437b84a47a8a gets rid of the isdir argument to fuse_file_put(), so this one liner that you disliked is gone. I will see if I can also get the opendir separation cleanup done. Thanks, Amir.
On Thu, Feb 1, 2024 at 6:46 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, Feb 1, 2024 at 12:51 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Thu, 1 Feb 2024 at 11:41, Amir Goldstein <amir73il@gmail.com> wrote: > > > > > I was considering splitting fuse_finish_open() to the first part that > > > can fail and the "finally" part that deals with attributes, but seeing > > > that this entire chunk of atomic_o_trunc code in fuse_finish_open() > > > is never relevant to atomic_open(), I'd rather just move it out > > > into fuse_open_common() which has loads of other code related to > > > atomic_o_trunc anyway? > > > > Yep. > > FWIW, I pushed some cleanups to: > > https://github.com/amir73il/linux/commits/fuse_io_mode-wip/ > > * e71b0c0356c8 - (github/fuse_io_mode-wip) fuse: introduce inode io modes > * 081ddd63a9ff - fuse: prepare for failing open response > * 437b84a47a8a - fuse: allocate ff->release_args only if release is needed > * e2df18f9a3d6 - fuse: factor out helper fuse_truncate_update_attr() > > e2df18f9a3d6 is the O_TRUNC change discussed above. > 437b84a47a8a gets rid of the isdir argument to fuse_file_put(), so this > one liner that you disliked is gone. > > I will see if I can also get the opendir separation cleanup done. > This is what I have in WIP branch - not sure if that is what you meant: * 285a83f439d8 - (github/fuse_io_mode-wip) fuse: introduce inode io modes * cf7e1707a319 - fuse: break up fuse_open_common() * d8fcee9252ca - fuse: prepare for failing open response * 5e4786da5d6e - fuse: allocate ff->release_args only if release is needed * 64a6a415239c - fuse: factor out helper fuse_truncate_update_attr() Thanks, Amir. commit cf7e1707a31990ed5df1294047909fce60cc3ec1 Author: Amir Goldstein <amir73il@gmail.com> Date: Fri Feb 2 13:30:30 2024 +0200 fuse: break up fuse_open_common() fuse_open_common() has a lot of code relevant only for regular files and O_TRUNC in particular. Copy the little bit of remaining code into fuse_dir_open() and stop using this common helper for directory open. Suggested-by: Miklos Szeredi <miklos@szeredi.hu> Signed-off-by: Amir Goldstein <amir73il@gmail.com> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 27daf0bf84ad..3498255402fe 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -1632,7 +1632,27 @@ static const char *fuse_get_link(struct dentry *dentry, struct inode *inode, static int fuse_dir_open(struct inode *inode, struct file *file) { - return fuse_open_common(inode, file, true); + struct fuse_mount *fm = get_fuse_mount(inode); + struct fuse_inode *fi = get_fuse_inode(inode); + int err; + + if (fuse_is_bad(inode)) + return -EIO; + + err = generic_file_open(inode, file); + if (err) + return err; + + err = fuse_do_open(fm, get_node_id(inode), file, true); + if (!err) { + struct fuse_file *ff = file->private_data; + + err = fuse_finish_open(inode, file); + if (err) + fuse_sync_release(fi, ff, file->f_flags); + } + + return err; } static int fuse_dir_release(struct inode *inode, struct file *file) diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 891bfa8a6724..1d6b3499c069 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -229,7 +229,7 @@ static void fuse_truncate_update_attr(struct inode *inode, struct file *file) fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE); } -int fuse_open_common(struct inode *inode, struct file *file, bool isdir) +int fuse_open(struct inode *inode, struct file *file) { struct fuse_mount *fm = get_fuse_mount(inode); struct fuse_inode *fi = get_fuse_inode(inode); @@ -260,7 +260,7 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir) if (is_wb_truncate || dax_truncate) fuse_set_nowrite(inode); - err = fuse_do_open(fm, get_node_id(inode), file, isdir); + err = fuse_do_open(fm, get_node_id(inode), file, false); if (!err) { ff = file->private_data; err = fuse_finish_open(inode, file); @@ -359,11 +359,6 @@ void fuse_release_common(struct file *file, bool isdir) (fl_owner_t) file, isdir); } -static int fuse_open(struct inode *inode, struct file *file) -{ - return fuse_open_common(inode, file, false); -} - static int fuse_release(struct inode *inode, struct file *file) { struct fuse_conn *fc = get_fuse_conn(inode); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 536b4515c2c8..9ad5f882bd0a 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -1034,8 +1034,6 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos, /** * Send OPEN or OPENDIR request */ -int fuse_open_common(struct inode *inode, struct file *file, bool isdir); - struct fuse_file *fuse_file_alloc(struct fuse_mount *fm, bool release); void fuse_file_free(struct fuse_file *ff); int fuse_finish_open(struct inode *inode, struct file *file); --
On Fri, 2 Feb 2024 at 13:03, Amir Goldstein <amir73il@gmail.com> wrote: > static int fuse_dir_open(struct inode *inode, struct file *file) > { > - return fuse_open_common(inode, file, true); > + struct fuse_mount *fm = get_fuse_mount(inode); > + struct fuse_inode *fi = get_fuse_inode(inode); > + int err; > + > + if (fuse_is_bad(inode)) > + return -EIO; > + > + err = generic_file_open(inode, file); > + if (err) > + return err; > + > + err = fuse_do_open(fm, get_node_id(inode), file, true); > + if (!err) { > + struct fuse_file *ff = file->private_data; > + > + err = fuse_finish_open(inode, file); I'd prefer fuse_finish_open() to be expanded as well. FMODE_WRITE is always false for directories. The other two FOPEN_ flags don't make sense for directories, but let's just leave them for a later cleanup. Thanks, Miklos
On Thu, 1 Feb 2024 at 00:09, Bernd Schubert <bschubert@ddn.com> wrote: > > From: Amir Goldstein <amir73il@gmail.com> > > In preparation for inode io modes, a server open response could fail > due to conflicting inode io modes. > > Allow returning an error from fuse_finish_open() and handle the error in > the callers. fuse_dir_open() can now call fuse_sync_release(), so handle > the isdir case correctly. Another question: will fuse_finish_open() fail for any reason other than due to an interrupted wait? If not, then maybe this belongs into a separate update which deals with killability/interruptibility in a comprehensive way. Thanks, Miklos
On Fri, Feb 2, 2024 at 5:55 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Thu, 1 Feb 2024 at 00:09, Bernd Schubert <bschubert@ddn.com> wrote: > > > > From: Amir Goldstein <amir73il@gmail.com> > > > > In preparation for inode io modes, a server open response could fail > > due to conflicting inode io modes. > > > > Allow returning an error from fuse_finish_open() and handle the error in > > the callers. fuse_dir_open() can now call fuse_sync_release(), so handle > > the isdir case correctly. > > Another question: will fuse_finish_open() fail for any reason other > than due to an interrupted wait? > Yes. Definitely. With fuse passthrough. Wait for parallel dio is Benrd's share of the patch set. My interest was always to deny cached open and passthrough open of the same inode and passthrough file open to a conflicting backing file as we have agreed [1]. See this passthough open commit for example: https://github.com/amir73il/linux/commit/9422b02931ca4be5471230645a2b4a6723f99d0e I will also split fuse_finish_dir_open() as you suggested. Thanks, Amir. [1] https://lore.kernel.org/linux-fsdevel/CAJfpegsoHtp_VthZRGfcoBREZ0pveb4wYYiKVEnCxaTgGEaeWw@mail.gmail.com/
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index d19cbf34c634..d45d4a678351 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -692,13 +692,15 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, d_instantiate(entry, inode); fuse_change_entry_timeout(entry, &outentry); fuse_dir_changed(dir); - err = finish_open(file, entry, generic_file_open); + err = generic_file_open(inode, file); + if (!err) { + file->private_data = ff; + err = finish_open(file, entry, fuse_finish_open); + } if (err) { fi = get_fuse_inode(inode); fuse_sync_release(fi, ff, flags); } else { - file->private_data = ff; - fuse_finish_open(inode, file); if (fm->fc->atomic_o_trunc && trunc) truncate_pagecache(inode, 0); else if (!(ff->open_flags & FOPEN_KEEP_CACHE)) diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 3062f4b5a34b..7d2f4b0eb36a 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -195,7 +195,7 @@ static void fuse_link_write_file(struct file *file) spin_unlock(&fi->lock); } -void fuse_finish_open(struct inode *inode, struct file *file) +int fuse_finish_open(struct inode *inode, struct file *file) { struct fuse_file *ff = file->private_data; struct fuse_conn *fc = get_fuse_conn(inode); @@ -217,12 +217,16 @@ void fuse_finish_open(struct inode *inode, struct file *file) } if ((file->f_mode & FMODE_WRITE) && fc->writeback_cache) fuse_link_write_file(file); + + return 0; } int fuse_open_common(struct inode *inode, struct file *file, bool isdir) { struct fuse_mount *fm = get_fuse_mount(inode); + struct fuse_inode *fi = get_fuse_inode(inode); struct fuse_conn *fc = fm->fc; + struct fuse_file *ff; int err; bool is_wb_truncate = (file->f_flags & O_TRUNC) && fc->atomic_o_trunc && @@ -251,14 +255,16 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir) fuse_set_nowrite(inode); err = fuse_do_open(fm, get_node_id(inode), file, isdir); - if (!err) - fuse_finish_open(inode, file); + if (!err) { + ff = file->private_data; + err = fuse_finish_open(inode, file); + if (err) + fuse_sync_release(fi, ff, file->f_flags); + } if (is_wb_truncate || dax_truncate) fuse_release_nowrite(inode); if (!err) { - struct fuse_file *ff = file->private_data; - if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC)) truncate_pagecache(inode, 0); else if (!(ff->open_flags & FOPEN_KEEP_CACHE)) @@ -368,7 +374,7 @@ void fuse_sync_release(struct fuse_inode *fi, struct fuse_file *ff, * iput(NULL) is a no-op and since the refcount is 1 and everything's * synchronous, we are fine with not doing igrab() here" */ - fuse_file_put(ff, true, false); + fuse_file_put(ff, true, fi && S_ISDIR(fi->inode.i_mode)); } EXPORT_SYMBOL_GPL(fuse_sync_release); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 1df83eebda92..1c0cde4022f0 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -1038,7 +1038,7 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir); struct fuse_file *fuse_file_alloc(struct fuse_mount *fm); void fuse_file_free(struct fuse_file *ff); -void fuse_finish_open(struct inode *inode, struct file *file); +int fuse_finish_open(struct inode *inode, struct file *file); void fuse_sync_release(struct fuse_inode *fi, struct fuse_file *ff, unsigned int flags);