| Message ID | 20230824150533.2788317-4-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse direct write consolidation and parallel IO | expand |
On Thu, 24 Aug 2023 at 17:08, Bernd Schubert <bschubert@ddn.com> wrote: > > Take a shared lock in fuse_cache_write_iter. > > 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 | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index a16f9b6888de..905ce3bb0047 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1314,9 +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; > > - return !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) || > - iocb->ki_flags & IOCB_APPEND || > - fuse_direct_write_extending_i_size(iocb, from); > + return ((!(iocb->ki_flags & IOCB_DIRECT)) || > + (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) || Why the extra parenthesis around the negation in the above two conditions? So this condition will always be true at this point when called from fuse_cache_write_iter()? If so, you need to explain in the commit message why are you doing this at this point (e.g. future patches depend on this). Thanks, Miklos
On 8/28/23 12:42, Miklos Szeredi wrote: > On Thu, 24 Aug 2023 at 17:08, Bernd Schubert <bschubert@ddn.com> wrote: >> >> Take a shared lock in fuse_cache_write_iter. >> >> 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 | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index a16f9b6888de..905ce3bb0047 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -1314,9 +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; >> >> - return !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) || >> - iocb->ki_flags & IOCB_APPEND || >> - fuse_direct_write_extending_i_size(iocb, from); >> + return ((!(iocb->ki_flags & IOCB_DIRECT)) || >> + (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) || > > Why the extra parenthesis around the negation in the above two conditions? > > So this condition will always be true at this point when called from > fuse_cache_write_iter()? If so, you need to explain in the commit > message why are you doing this at this point (e.g. future patches > depend on this). Oh, thanks for spotting, the double parenthesis were accidentally. Although I don't think it would have an effect, it just results in return ((!(condition1)) || ... I.e. does not change the condition itself? Anyway, yeah, agreed on your comment in the patch before, with one condition per line it becomes easier to read and avoids parenthesis. I had just tried to keep the code as it is to make the patch easier to read. Thanks, Bernd
On Mon, 28 Aug 2023 at 16:21, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 8/28/23 12:42, Miklos Szeredi wrote: > > On Thu, 24 Aug 2023 at 17:08, Bernd Schubert <bschubert@ddn.com> wrote: > >> > >> Take a shared lock in fuse_cache_write_iter. > >> > >> 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 | 21 ++++++++++++++++----- > >> 1 file changed, 16 insertions(+), 5 deletions(-) > >> > >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c > >> index a16f9b6888de..905ce3bb0047 100644 > >> --- a/fs/fuse/file.c > >> +++ b/fs/fuse/file.c > >> @@ -1314,9 +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; > >> > >> - return !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) || > >> - iocb->ki_flags & IOCB_APPEND || > >> - fuse_direct_write_extending_i_size(iocb, from); > >> + return ((!(iocb->ki_flags & IOCB_DIRECT)) || > >> + (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) || > > > > Why the extra parenthesis around the negation in the above two conditions? > > > > So this condition will always be true at this point when called from > > fuse_cache_write_iter()? If so, you need to explain in the commit > > message why are you doing this at this point (e.g. future patches > > depend on this). > > Oh, thanks for spotting, the double parenthesis were accidentally. > Although I don't think it would have an effect, it just results in > > return ((!(condition1)) || ... > > I.e. does not change the condition itself? It doesn't change the condition, but degrades readability. Thanks, Miklos
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index a16f9b6888de..905ce3bb0047 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1314,9 +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; - return !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) || - iocb->ki_flags & IOCB_APPEND || - fuse_direct_write_extending_i_size(iocb, from); + return ((!(iocb->ki_flags & IOCB_DIRECT)) || + (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) || + iocb->ki_flags & IOCB_APPEND || + fuse_direct_write_extending_i_size(iocb, from)); } static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) @@ -1327,6 +1328,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) */ @@ -1345,7 +1347,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) @@ -1360,6 +1365,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; @@ -1369,7 +1377,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. 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 | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)