[PATCH] fuse: add fast path for fuse_range_is_writeback

[PATCH] fuse: add fast path for fuse_range_is_writeback

[yangyun]

In some cases, the fi->writepages may be empty. And there is no need
to check fi->writepages with spin_lock, which may have an impact on
performance due to lock contention. For example, in scenarios where
multiple readers read the same file without any writers, or where
the page cache is not enabled.

Also remove the outdated comment since commit 6b2fb79963fb ("fuse:
optimize writepages search") has optimize the situation by replacing
list with rb-tree.

Signed-off-by: yangyun <yangyun50@huawei.com>
---
 fs/fuse/file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index f39456c65ed7..59c911b61000 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -448,9 +448,6 @@ static struct fuse_writepage_args *fuse_find_writeback(struct fuse_inode *fi,
 
 /*
  * Check if any page in a range is under writeback
- *
- * This is currently done by walking the list of writepage requests
- * for the inode, which can be pretty inefficient.
  */
 static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
 				   pgoff_t idx_to)
@@ -458,6 +455,9 @@ static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	bool found;
 
+	if (RB_EMPTY_ROOT(&fi->writepages))
+		return false;
+
 	spin_lock(&fi->lock);
 	found = fuse_find_writeback(fi, idx_from, idx_to);
 	spin_unlock(&fi->lock);
-- 
2.33.0

Re: [PATCH] fuse: add fast path for fuse_range_is_writeback

[Jingbo Xu]

On 8/14/24 5:36 PM, yangyun wrote:
> In some cases, the fi->writepages may be empty. And there is no need
> to check fi->writepages with spin_lock, which may have an impact on
> performance due to lock contention. For example, in scenarios where
> multiple readers read the same file without any writers, or where
> the page cache is not enabled.
> 
> Also remove the outdated comment since commit 6b2fb79963fb ("fuse:
> optimize writepages search") has optimize the situation by replacing
> list with rb-tree.
> 
> Signed-off-by: yangyun <yangyun50@huawei.com>
> ---
>  fs/fuse/file.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index f39456c65ed7..59c911b61000 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -448,9 +448,6 @@ static struct fuse_writepage_args *fuse_find_writeback(struct fuse_inode *fi,
>  
>  /*
>   * Check if any page in a range is under writeback
> - *
> - * This is currently done by walking the list of writepage requests
> - * for the inode, which can be pretty inefficient.
>   */
>  static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
>  				   pgoff_t idx_to)
> @@ -458,6 +455,9 @@ static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
>  	struct fuse_inode *fi = get_fuse_inode(inode);
>  	bool found;
>  
> +	if (RB_EMPTY_ROOT(&fi->writepages))
> +		return false;

fi->lock is held when inserting wpa into fi->writepages rbtree (see
fuse_writepage_add()).  I doubt if there is race condition when checking
if fi->writepages rbtree is empty without fi->lock held.


-- 
Thanks,
Jingbo

Re: [PATCH] fuse: add fast path for fuse_range_is_writeback

[Miklos Szeredi]

On Wed, 14 Aug 2024 at 11:36, yangyun <yangyun50@huawei.com> wrote:
>
> In some cases, the fi->writepages may be empty. And there is no need
> to check fi->writepages with spin_lock, which may have an impact on
> performance due to lock contention. For example, in scenarios where
> multiple readers read the same file without any writers, or where
> the page cache is not enabled.
>
> Also remove the outdated comment since commit 6b2fb79963fb ("fuse:
> optimize writepages search") has optimize the situation by replacing
> list with rb-tree.

Thanks, applied.

Miklos

Re: [PATCH] fuse: add fast path for fuse_range_is_writeback

[yangyun]

Sorry for the late reply.

On Wed, Aug 14, 2024 at 05:56:06PM +0800, Jingbo Xu wrote:
> 
> 
> On 8/14/24 5:36 PM, yangyun wrote:
> > In some cases, the fi->writepages may be empty. And there is no need
> > to check fi->writepages with spin_lock, which may have an impact on
> > performance due to lock contention. For example, in scenarios where
> > multiple readers read the same file without any writers, or where
> > the page cache is not enabled.
> > 
> > Also remove the outdated comment since commit 6b2fb79963fb ("fuse:
> > optimize writepages search") has optimize the situation by replacing
> > list with rb-tree.
> > 
> > Signed-off-by: yangyun <yangyun50@huawei.com>
> > ---
> >  fs/fuse/file.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index f39456c65ed7..59c911b61000 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -448,9 +448,6 @@ static struct fuse_writepage_args *fuse_find_writeback(struct fuse_inode *fi,
> >  
> >  /*
> >   * Check if any page in a range is under writeback
> > - *
> > - * This is currently done by walking the list of writepage requests
> > - * for the inode, which can be pretty inefficient.
> >   */
> >  static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
> >  				   pgoff_t idx_to)
> > @@ -458,6 +455,9 @@ static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
> >  	struct fuse_inode *fi = get_fuse_inode(inode);
> >  	bool found;
> >  
> > +	if (RB_EMPTY_ROOT(&fi->writepages))
> > +		return false;
> 
> fi->lock is held when inserting wpa into fi->writepages rbtree (see
> fuse_writepage_add()).  I doubt if there is race condition when checking
> if fi->writepages rbtree is empty without fi->lock held.

The code can make sure that there are no race conditions because:
1. For O_DIRECT and FOPEN_DIRECT_IO with fc->direct_io_allow_mmap, the `filemap_write_and_wait_range` before can make the insert operation to be happend before the check operation.
2. For other cases, there are no pagecache operaions so the fi->writepages is always empty.

In my usercase, the fi->writepages is usually empty but the spin_lock associated with it contributes a great impact on the performace of my filesystem due to lock contention. So optimize it.
> 
> -- 
> Thanks,
> Jingbo

Re: [PATCH] fuse: add fast path for fuse_range_is_writeback

[Jingbo Xu]

On 8/23/24 2:19 PM, yangyun wrote:
> Sorry for the late reply.
> 
> On Wed, Aug 14, 2024 at 05:56:06PM +0800, Jingbo Xu wrote:
>>
>>
>> On 8/14/24 5:36 PM, yangyun wrote:
>>> In some cases, the fi->writepages may be empty. And there is no need
>>> to check fi->writepages with spin_lock, which may have an impact on
>>> performance due to lock contention. For example, in scenarios where
>>> multiple readers read the same file without any writers, or where
>>> the page cache is not enabled.
>>>
>>> Also remove the outdated comment since commit 6b2fb79963fb ("fuse:
>>> optimize writepages search") has optimize the situation by replacing
>>> list with rb-tree.
>>>
>>> Signed-off-by: yangyun <yangyun50@huawei.com>
>>> ---
>>>  fs/fuse/file.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index f39456c65ed7..59c911b61000 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -448,9 +448,6 @@ static struct fuse_writepage_args *fuse_find_writeback(struct fuse_inode *fi,
>>>  
>>>  /*
>>>   * Check if any page in a range is under writeback
>>> - *
>>> - * This is currently done by walking the list of writepage requests
>>> - * for the inode, which can be pretty inefficient.
>>>   */
>>>  static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
>>>  				   pgoff_t idx_to)
>>> @@ -458,6 +455,9 @@ static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
>>>  	struct fuse_inode *fi = get_fuse_inode(inode);
>>>  	bool found;
>>>  
>>> +	if (RB_EMPTY_ROOT(&fi->writepages))
>>> +		return false;
>>
>> fi->lock is held when inserting wpa into fi->writepages rbtree (see
>> fuse_writepage_add()).  I doubt if there is race condition when checking
>> if fi->writepages rbtree is empty without fi->lock held.
> 
> The code can make sure that there are no race conditions because:
> 1. For O_DIRECT and FOPEN_DIRECT_IO with fc->direct_io_allow_mmap, the `filemap_write_and_wait_range` before can make the insert operation to be happend before the check operation.
> 2. For other cases, there are no pagecache operaions so the fi->writepages is always empty.
> 
> In my usercase, the fi->writepages is usually empty but the spin_lock associated with it contributes a great impact on the performace of my filesystem due to lock contention. So optimize it.

Thanks for the explanation.  There are multiple callsites of
fuse_wait_on_page_writeback(), and how do you ensure that there can't be
any concurrent fuse_writepage_add() with these callsites.

I'm not sure if the concurrency indeed makes a difference, since
fuse_range_is_writeback() releases fi->lock before it returns anyway.


-- 
Thanks,
Jingbo