| Message ID | 20240131230827.207552-3-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: inode IO modes and mmap | expand |
On 2/1/24 7:08 AM, Bernd Schubert wrote: > @@ -1591,10 +1616,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) > else { > inode_lock_shared(inode); > > - /* A race with truncate might have come up as the decision for > - * the lock type was done without holding the lock, check again. > + /* > + * Previous check was without any lock and might have raced. > */ > - if (fuse_direct_write_extending_i_size(iocb, from)) { > + if (fuse_dio_wr_exclusive_lock(iocb, from)) { ^ The overall is good. Maybe fuse_io_past_eof() is better to make it a solely cleanup or refactoring. Actually it's already changed back to fuse_io_past_eof() in patch 3/5.
On 2/6/24 10:20, Jingbo Xu wrote: > > > On 2/1/24 7:08 AM, Bernd Schubert wrote: >> @@ -1591,10 +1616,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) >> else { >> inode_lock_shared(inode); >> >> - /* A race with truncate might have come up as the decision for >> - * the lock type was done without holding the lock, check again. >> + /* >> + * Previous check was without any lock and might have raced. >> */ >> - if (fuse_direct_write_extending_i_size(iocb, from)) { >> + if (fuse_dio_wr_exclusive_lock(iocb, from)) { > ^ > > The overall is good. Maybe fuse_io_past_eof() is better to make it a > solely cleanup or refactoring. Actually it's already changed back to > fuse_io_past_eof() in patch 3/5. So I'm bit confused what you would like to see improved. Patch 2/5 renames "fuse_direct_write_extending_i_size" to "fuse_io_past_eof" and also moves it up in the file. (The latter is a preparation for my direct-write consolidation patches.). It also creates the helper function fuse_dio_wr_exclusive_lock(). None of that is changed in 3/5, which just moves the locking/unlocking from fuse_cache_write_iter() into the functions fuse_dio_lock/fuse_dio_unlock. Thanks, Bernd
On 2/7/24 9:38 PM, Bernd Schubert wrote: > > > On 2/6/24 10:20, Jingbo Xu wrote: >> >> >> On 2/1/24 7:08 AM, Bernd Schubert wrote: >>> @@ -1591,10 +1616,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) >>> else { >>> inode_lock_shared(inode); >>> >>> - /* A race with truncate might have come up as the decision for >>> - * the lock type was done without holding the lock, check again. >>> + /* >>> + * Previous check was without any lock and might have raced. >>> */ >>> - if (fuse_direct_write_extending_i_size(iocb, from)) { >>> + if (fuse_dio_wr_exclusive_lock(iocb, from)) { >> ^ I mean, in patch 2/5 > - if (fuse_direct_write_extending_i_size(iocb, from)) { > + if (fuse_io_past_eof(iocb, from)) { is better, otherwise it's not an equivalent change. Thanks, Jingbo
On 2/7/24 14:44, Jingbo Xu wrote: > > > On 2/7/24 9:38 PM, Bernd Schubert wrote: >> >> >> On 2/6/24 10:20, Jingbo Xu wrote: >>> >>> >>> On 2/1/24 7:08 AM, Bernd Schubert wrote: >>>> @@ -1591,10 +1616,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) >>>> else { >>>> inode_lock_shared(inode); >>>> >>>> - /* A race with truncate might have come up as the decision for >>>> - * the lock type was done without holding the lock, check again. >>>> + /* >>>> + * Previous check was without any lock and might have raced. >>>> */ > > >>>> - if (fuse_direct_write_extending_i_size(iocb, from)) { >>>> + if (fuse_dio_wr_exclusive_lock(iocb, from)) { >>> ^ > > I mean, in patch 2/5 > >> - if (fuse_direct_write_extending_i_size(iocb, from)) { >> + if (fuse_io_past_eof(iocb, from)) { > > is better, otherwise it's not an equivalent change. Ah thanks, good catch! Now I see what you mean. Yeah, we can switch to fuse_io_past_eof() here. And yeah, 3/5 changes it back. Fortunately there is actually not much harm, as fuse_dio_wr_exclusive_lock also calls into fuse_io_past_eof. Thanks, Bernd
On 2/7/24 10:13 PM, Bernd Schubert wrote: > > > On 2/7/24 14:44, Jingbo Xu wrote: >> >> >> On 2/7/24 9:38 PM, Bernd Schubert wrote: >>> >>> >>> On 2/6/24 10:20, Jingbo Xu wrote: >>>> >>>> >>>> On 2/1/24 7:08 AM, Bernd Schubert wrote: >>>>> @@ -1591,10 +1616,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) >>>>> else { >>>>> inode_lock_shared(inode); >>>>> >>>>> - /* A race with truncate might have come up as the decision for >>>>> - * the lock type was done without holding the lock, check again. >>>>> + /* >>>>> + * Previous check was without any lock and might have raced. >>>>> */ >> >> >>>>> - if (fuse_direct_write_extending_i_size(iocb, from)) { >>>>> + if (fuse_dio_wr_exclusive_lock(iocb, from)) { >>>> ^ >> >> I mean, in patch 2/5 >> >>> - if (fuse_direct_write_extending_i_size(iocb, from)) { >>> + if (fuse_io_past_eof(iocb, from)) { >> >> is better, otherwise it's not an equivalent change. > > Ah thanks, good catch! Now I see what you mean. Yeah, we can switch to > fuse_io_past_eof() here. And yeah, 3/5 changes it back. > Fortunately there is actually not much harm, as > fuse_dio_wr_exclusive_lock also calls into fuse_io_past_eof. > Yeah fortunately we needn't retest it as patch 3/5 changes it back. The whole series is good. The comment is just from per-patch basis. Thanks, Jingbo
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 243f469cac07..0c4d93293eac 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1299,6 +1299,45 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii) return res; } +static bool fuse_io_past_eof(struct kiocb *iocb, struct iov_iter *iter) +{ + struct inode *inode = file_inode(iocb->ki_filp); + + return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode); +} + +/* + * @return true if an exclusive lock for direct IO writes is needed + */ +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; + struct inode *inode = file_inode(iocb->ki_filp); + + /* server side has to advise that it supports parallel dio writes */ + if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) + return true; + + /* append will need to know the eventual eof - always needs an + * exclusive lock + */ + if (iocb->ki_flags & IOCB_APPEND) + return true; + + /* combination opf page access and direct-io difficult, shared + * locks actually introduce a conflict. + */ + if (get_fuse_conn(inode)->direct_io_allow_mmap) + return true; + + /* parallel dio beyond eof is at least for now not supported */ + if (fuse_io_past_eof(iocb, from)) + return true; + + return false; +} + static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct file *file = iocb->ki_filp; @@ -1558,26 +1597,12 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to) return res; } -static bool fuse_direct_write_extending_i_size(struct kiocb *iocb, - struct iov_iter *iter) -{ - struct inode *inode = file_inode(iocb->ki_filp); - - return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode); -} - static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct inode *inode = file_inode(iocb->ki_filp); - struct file *file = iocb->ki_filp; - struct fuse_file *ff = file->private_data; struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb); ssize_t res; - bool exclusive_lock = - !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) || - get_fuse_conn(inode)->direct_io_allow_mmap || - iocb->ki_flags & IOCB_APPEND || - fuse_direct_write_extending_i_size(iocb, from); + bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from); /* * Take exclusive lock if @@ -1591,10 +1616,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) else { inode_lock_shared(inode); - /* A race with truncate might have come up as the decision for - * the lock type was done without holding the lock, check again. + /* + * Previous check was without any lock and might have raced. */ - if (fuse_direct_write_extending_i_size(iocb, from)) { + if (fuse_dio_wr_exclusive_lock(iocb, from)) { inode_unlock_shared(inode); inode_lock(inode); exclusive_lock = true; @@ -2468,7 +2493,8 @@ static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma) return fuse_dax_mmap(file, vma); if (ff->open_flags & FOPEN_DIRECT_IO) { - /* Can't provide the coherency needed for MAP_SHARED + /* + * Can't provide the coherency needed for MAP_SHARED * if FUSE_DIRECT_IO_ALLOW_MMAP isn't set. */ if ((vma->vm_flags & VM_MAYSHARE) && !fc->direct_io_allow_mmap)