| Message ID | 20230829161116.2914040-4-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse direct write consolidation and parallel IO | expand |
On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote: > > Take a shared lock in fuse_cache_write_iter. This was already > done for FOPEN_DIRECT_IO in > > commit 153524053bbb ("fuse: allow non-extending parallel direct > writes on the same file") > > but so far missing for plain O_DIRECT. Server side needs > to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that > it supports parallel dio writes. Hmm, I think file_remove_privs() needs exclusive lock (due to calling notify_change()). And the fallback case also. Need to be careful with such locking changes... Thanks, Miklos
On 8/30/23 15:28, Miklos Szeredi wrote: > On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote: >> >> Take a shared lock in fuse_cache_write_iter. This was already >> done for FOPEN_DIRECT_IO in >> >> commit 153524053bbb ("fuse: allow non-extending parallel direct >> writes on the same file") >> >> but so far missing for plain O_DIRECT. Server side needs >> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that >> it supports parallel dio writes. > > Hmm, I think file_remove_privs() needs exclusive lock (due to calling > notify_change()). And the fallback case also. > > Need to be careful with such locking changes... Hrmm, yeah, I missed that :( Really sorry and thanks! I guess I can fix it if by exporting dentry_needs_remove_privs. I hope that is acceptable. Interesting is that btrfs_direct_write seems to have the same issue. btrfs_direct_write if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode)) ilock_flags |= BTRFS_ILOCK_SHARED; ... err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags); ... err = btrfs_write_check(iocb, from, err); ... ret = file_remove_privs(file); I think that can be fixed as well after exporting dentry_needs_remove_privs(). Btw, why is fuse_direct_write_iter not dropping privileges? That is another change I need to document... Another issue I just see is that it needs to check file size again after taking the lock. Thanks a lot for your review, Bernd
On Wed, 30 Aug 2023 at 16:38, Bernd Schubert <aakef@fastmail.fm> wrote: > > > > On 8/30/23 15:28, Miklos Szeredi wrote: > > On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote: > >> > >> Take a shared lock in fuse_cache_write_iter. This was already > >> done for FOPEN_DIRECT_IO in > >> > >> commit 153524053bbb ("fuse: allow non-extending parallel direct > >> writes on the same file") > >> > >> but so far missing for plain O_DIRECT. Server side needs > >> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that > >> it supports parallel dio writes. > > > > Hmm, I think file_remove_privs() needs exclusive lock (due to calling > > notify_change()). And the fallback case also. > > > > Need to be careful with such locking changes... > > Hrmm, yeah, I missed that :( Really sorry and thanks! > > I guess I can fix it if by exporting dentry_needs_remove_privs. I hope > that is acceptable. Interesting is that btrfs_direct_write seems to have > the same issue. > > btrfs_direct_write > if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode)) > ilock_flags |= BTRFS_ILOCK_SHARED; > ... > err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags); > ... > err = btrfs_write_check(iocb, from, err); > ... > ret = file_remove_privs(file); > > > I think that can be fixed as well after exporting > dentry_needs_remove_privs(). Interesting. Would be worth asking the btrfs devs what they think of this. > > > Btw, why is fuse_direct_write_iter not dropping privileges? That is > another change I need to document... Generally read and write can't fail due to lack of privileges, so that should be okay. But it would be more consistent to drop privs during I/O as well. But this is something that needs some thought as well, because there could be non-obvious consequences. > Another issue I just see is that it needs to check file size again after > taking the lock. Yes. > Thanks a lot for your review, Thanks for doing the actual work, that's the harder one. Thanks, Miklos
On 8/30/23 00:11, Bernd Schubert wrote: > Take a shared lock in fuse_cache_write_iter. This was already > done for FOPEN_DIRECT_IO in > > commit 153524053bbb ("fuse: allow non-extending parallel direct > writes on the same file") > > but so far missing for plain O_DIRECT. Server side needs > to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that > it supports parallel dio writes. > > Cc: Hao Xu <howeyxu@tencent.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Dharmendra Singh <dsingh@ddn.com> > Cc: linux-fsdevel@vger.kernel.org > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > --- > fs/fuse/file.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 6b8b9512c336..a6b99bc80fe7 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1314,6 +1314,10 @@ static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from > struct file *file = iocb->ki_filp; > struct fuse_file *ff = file->private_data; > > + /* this function is about direct IO only */ > + if (!(iocb->ki_flags & IOCB_DIRECT)) > + return false; This means for buffered write in fuse_cache_write_iter(), we grab shared lock, looks not right. > + > /* server side has to advise that it supports parallel dio writes */ > if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) > return false; > @@ -1337,6 +1341,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) > struct inode *inode = mapping->host; > ssize_t err; > struct fuse_conn *fc = get_fuse_conn(inode); > + bool excl_lock = fuse_dio_wr_exclusive_lock(iocb, from); > > if (fc->writeback_cache && !(iocb->ki_flags & IOCB_DIRECT)) { > /* Update size (EOF optimization) and mode (SUID clearing) */ > @@ -1355,7 +1360,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) > } > > writethrough: > - inode_lock(inode); > + if (excl_lock) > + inode_lock(inode); > + else > + inode_lock_shared(inode); > > err = generic_write_checks(iocb, from); > if (err <= 0) > @@ -1370,6 +1378,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) > goto out; > > if (iocb->ki_flags & IOCB_DIRECT) { > + /* file extending writes will trigger i_size_write - exclusive > + * lock is needed > + */ > written = generic_file_direct_write(iocb, from); > if (written < 0 || !iov_iter_count(from)) > goto out; > @@ -1379,7 +1390,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) > written = fuse_perform_write(iocb, from); > } > out: > - inode_unlock(inode); > + if (excl_lock) > + inode_unlock(inode); > + else > + inode_unlock_shared(inode); > if (written > 0) > written = generic_write_sync(iocb, written); >
On 8/31/23 10:30, Hao Xu wrote: > On 8/30/23 00:11, Bernd Schubert wrote: >> Take a shared lock in fuse_cache_write_iter. This was already >> done for FOPEN_DIRECT_IO in >> >> commit 153524053bbb ("fuse: allow non-extending parallel direct >> writes on the same file") >> >> but so far missing for plain O_DIRECT. Server side needs >> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that >> it supports parallel dio writes. >> >> Cc: Hao Xu <howeyxu@tencent.com> >> Cc: Miklos Szeredi <miklos@szeredi.hu> >> Cc: Dharmendra Singh <dsingh@ddn.com> >> Cc: linux-fsdevel@vger.kernel.org >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> --- >> fs/fuse/file.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index 6b8b9512c336..a6b99bc80fe7 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -1314,6 +1314,10 @@ static bool fuse_dio_wr_exclusive_lock(struct >> kiocb *iocb, struct iov_iter *from >> struct file *file = iocb->ki_filp; >> struct fuse_file *ff = file->private_data; >> + /* this function is about direct IO only */ >> + if (!(iocb->ki_flags & IOCB_DIRECT)) >> + return false; > > This means for buffered write in fuse_cache_write_iter(), we grab shared > lock, looks not right. > Yeah, sorry, I made all values the other way around, consistently. Miklos had already noticed. Thanks, Bernd
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 6b8b9512c336..a6b99bc80fe7 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1314,6 +1314,10 @@ static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from struct file *file = iocb->ki_filp; struct fuse_file *ff = file->private_data; + /* this function is about direct IO only */ + if (!(iocb->ki_flags & IOCB_DIRECT)) + return false; + /* server side has to advise that it supports parallel dio writes */ if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) return false; @@ -1337,6 +1341,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) struct inode *inode = mapping->host; ssize_t err; struct fuse_conn *fc = get_fuse_conn(inode); + bool excl_lock = fuse_dio_wr_exclusive_lock(iocb, from); if (fc->writeback_cache && !(iocb->ki_flags & IOCB_DIRECT)) { /* Update size (EOF optimization) and mode (SUID clearing) */ @@ -1355,7 +1360,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) } writethrough: - inode_lock(inode); + if (excl_lock) + inode_lock(inode); + else + inode_lock_shared(inode); err = generic_write_checks(iocb, from); if (err <= 0) @@ -1370,6 +1378,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) goto out; if (iocb->ki_flags & IOCB_DIRECT) { + /* file extending writes will trigger i_size_write - exclusive + * lock is needed + */ written = generic_file_direct_write(iocb, from); if (written < 0 || !iov_iter_count(from)) goto out; @@ -1379,7 +1390,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) written = fuse_perform_write(iocb, from); } out: - inode_unlock(inode); + if (excl_lock) + inode_unlock(inode); + else + inode_unlock_shared(inode); if (written > 0) written = generic_write_sync(iocb, written);
Take a shared lock in fuse_cache_write_iter. This was already done for FOPEN_DIRECT_IO in commit 153524053bbb ("fuse: allow non-extending parallel direct writes on the same file") but so far missing for plain O_DIRECT. Server side needs to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that it supports parallel dio writes. Cc: Hao Xu <howeyxu@tencent.com> Cc: Miklos Szeredi <miklos@szeredi.hu> Cc: Dharmendra Singh <dsingh@ddn.com> Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/file.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)