[PATCH 1/2] fuse: drop unused fuse_mount arg in fuse_writepage_finish

[PATCH 1/2] fuse: drop unused fuse_mount arg in fuse_writepage_finish

[Joanne Koong]

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index f39456c65ed7..63fd5fc6872e 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1769,8 +1769,7 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa)
 	kfree(wpa);
 }
 
-static void fuse_writepage_finish(struct fuse_mount *fm,
-				  struct fuse_writepage_args *wpa)
+static void fuse_writepage_finish(struct fuse_writepage_args *wpa)
 {
 	struct fuse_args_pages *ap = &wpa->ia.ap;
 	struct inode *inode = wpa->inode;
@@ -1829,7 +1828,7 @@ __acquires(fi->lock)
  out_free:
 	fi->writectr--;
 	rb_erase(&wpa->writepages_entry, &fi->writepages);
-	fuse_writepage_finish(fm, wpa);
+	fuse_writepage_finish(wpa);
 	spin_unlock(&fi->lock);
 
 	/* After fuse_writepage_finish() aux request list is private */
@@ -1959,7 +1958,7 @@ static void fuse_writepage_end(struct fuse_mount *fm, struct fuse_args *args,
 		fuse_send_writepage(fm, next, inarg->offset + inarg->size);
 	}
 	fi->writectr--;
-	fuse_writepage_finish(fm, wpa);
+	fuse_writepage_finish(wpa);
 	spin_unlock(&fi->lock);
 	fuse_writepage_free(wpa);
 }
-- 
2.43.5

[PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Joanne Koong]

In the case where the aux writeback list is dropped (eg the pages
have been truncated or the connection is broken), the stats for
its pages and backing device info need to be updated as well.

Fixes: e2653bd53a98 ("fuse: fix leaked aux requests")
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 63fd5fc6872e..7ac56be5fee6 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1831,10 +1831,11 @@ __acquires(fi->lock)
 	fuse_writepage_finish(wpa);
 	spin_unlock(&fi->lock);
 
-	/* After fuse_writepage_finish() aux request list is private */
+	/* After rb_erase() aux request list is private */
 	for (aux = wpa->next; aux; aux = next) {
 		next = aux->next;
 		aux->next = NULL;
+		fuse_writepage_finish(aux);
 		fuse_writepage_free(aux);
 	}
 
-- 
2.43.5

Re: [PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Jingbo Xu]

On 8/20/24 2:24 AM, Joanne Koong wrote:
> In the case where the aux writeback list is dropped (eg the pages
> have been truncated or the connection is broken), the stats for
> its pages and backing device info need to be updated as well.
> 
> Fixes: e2653bd53a98 ("fuse: fix leaked aux requests")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 63fd5fc6872e..7ac56be5fee6 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1831,10 +1831,11 @@ __acquires(fi->lock)
>  	fuse_writepage_finish(wpa);
>  	spin_unlock(&fi->lock);
>  
> -	/* After fuse_writepage_finish() aux request list is private */
> +	/* After rb_erase() aux request list is private */
>  	for (aux = wpa->next; aux; aux = next) {
>  		next = aux->next;
>  		aux->next = NULL;
> +		fuse_writepage_finish(aux);
>  		fuse_writepage_free(aux);
>  	}
>  

LGTM.

Besides, there is similar logic of decreasing stats info for replaced
aux (temp) request inside fuse_writepage_add(), though without waking up
fi->page_waitq.

I wonder if we could factor out a new helper function, saying
fuse_writepage_dec_stat(), which could be called both from
fuse_writepage_add() and fuse_send_writepage().


-- 
Thanks,
Jingbo

Re: [PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Joanne Koong]

On Mon, Aug 19, 2024 at 7:10 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
>
>
> On 8/20/24 2:24 AM, Joanne Koong wrote:
> > In the case where the aux writeback list is dropped (eg the pages
> > have been truncated or the connection is broken), the stats for
> > its pages and backing device info need to be updated as well.
> >
> > Fixes: e2653bd53a98 ("fuse: fix leaked aux requests")
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/file.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 63fd5fc6872e..7ac56be5fee6 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -1831,10 +1831,11 @@ __acquires(fi->lock)
> >       fuse_writepage_finish(wpa);
> >       spin_unlock(&fi->lock);
> >
> > -     /* After fuse_writepage_finish() aux request list is private */
> > +     /* After rb_erase() aux request list is private */
> >       for (aux = wpa->next; aux; aux = next) {
> >               next = aux->next;
> >               aux->next = NULL;
> > +             fuse_writepage_finish(aux);
> >               fuse_writepage_free(aux);
> >       }
> >
>
> LGTM.
>
> Besides, there is similar logic of decreasing stats info for replaced
> aux (temp) request inside fuse_writepage_add(), though without waking up
> fi->page_waitq.
>
> I wonder if we could factor out a new helper function, saying
> fuse_writepage_dec_stat(), which could be called both from
> fuse_writepage_add() and fuse_send_writepage().

This sounds good to me. I'll add this refactoring when I resubmit this
patch series.

Thanks,
Joanne

>
>
> --
> Thanks,
> Jingbo

Re: [PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Miklos Szeredi]

On Mon, 19 Aug 2024 at 20:25, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> In the case where the aux writeback list is dropped (eg the pages
> have been truncated or the connection is broken), the stats for
> its pages and backing device info need to be updated as well.

Patch looks good.  Thanks.

Do you have a reproducer or was this found by code review only?

Thanks,
Miklos

Re: [PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Bernd Schubert]

On 8/21/24 17:56, Miklos Szeredi wrote:
> On Mon, 19 Aug 2024 at 20:25, Joanne Koong <joannelkoong@gmail.com> wrote:
>>
>> In the case where the aux writeback list is dropped (eg the pages
>> have been truncated or the connection is broken), the stats for
>> its pages and backing device info need to be updated as well.
> 
> Patch looks good.  Thanks.
> 
> Do you have a reproducer or was this found by code review only?

That's indeed a nice catch from Joanne!.

I would have expected that writing to a file and in parallel truncating
it would leak WritebackTmp in /proc/meminfo. But I see it going up and
always to 0 again.


Thanks,
Bernd

Re: [PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Joanne Koong]

On Wed, Aug 21, 2024 at 11:26 AM Bernd Schubert
<bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/21/24 17:56, Miklos Szeredi wrote:
> > On Mon, 19 Aug 2024 at 20:25, Joanne Koong <joannelkoong@gmail.com> wrote:
> >>
> >> In the case where the aux writeback list is dropped (eg the pages
> >> have been truncated or the connection is broken), the stats for
> >> its pages and backing device info need to be updated as well.
> >
> > Patch looks good.  Thanks.
> >
> > Do you have a reproducer or was this found by code review only?

Hi Miklos!

I unfortunately don't have a repro, this was found by code review. I
started looking at the writeback code after reading through this
thread
https://lore.kernel.org/all/495d2400-1d96-4924-99d3-8b2952e05fc3@linux.alibaba.com/#t

For v2 of this patchset, I am planning to add a few more refactoring
patches like abstracting out the shared logic between
fuse_writepages_fill() and fuse_writepage_locked(). My plan is to
submit v2 this week.

>
> That's indeed a nice catch from Joanne!.
>
> I would have expected that writing to a file and in parallel truncating
> it would leak WritebackTmp in /proc/meminfo. But I see it going up and
> always to 0 again.

I think we only hit this leaked case when we're writing back to a page
that is already in writeback (which then leads it to being placed on
the auxiliary list). I think in your example, the page isn't already
in writeback?

>
>
> Thanks,
> Bernd

Thanks,
Joanne

Re: [PATCH 2/2] fuse: update stats for pages in dropped aux writeback list

[Miklos Szeredi]

On Wed, 21 Aug 2024 at 22:22, Joanne Koong <joannelkoong@gmail.com> wrote:

> I unfortunately don't have a repro, this was found by code review. I
> started looking at the writeback code after reading through this
> thread
> https://lore.kernel.org/all/495d2400-1d96-4924-99d3-8b2952e05fc3@linux.alibaba.com/#t

No problem, I was just curious.

Thanks,
Miklos

Re: [PATCH 1/2] fuse: drop unused fuse_mount arg in fuse_writepage_finish

[Jingbo Xu]

On 8/20/24 2:24 AM, Joanne Koong wrote:
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index f39456c65ed7..63fd5fc6872e 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1769,8 +1769,7 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa)
>  	kfree(wpa);
>  }
>  
> -static void fuse_writepage_finish(struct fuse_mount *fm,
> -				  struct fuse_writepage_args *wpa)
> +static void fuse_writepage_finish(struct fuse_writepage_args *wpa)
>  {
>  	struct fuse_args_pages *ap = &wpa->ia.ap;
>  	struct inode *inode = wpa->inode;
> @@ -1829,7 +1828,7 @@ __acquires(fi->lock)
>   out_free:
>  	fi->writectr--;
>  	rb_erase(&wpa->writepages_entry, &fi->writepages);
> -	fuse_writepage_finish(fm, wpa);
> +	fuse_writepage_finish(wpa);
>  	spin_unlock(&fi->lock);
>  
>  	/* After fuse_writepage_finish() aux request list is private */
> @@ -1959,7 +1958,7 @@ static void fuse_writepage_end(struct fuse_mount *fm, struct fuse_args *args,
>  		fuse_send_writepage(fm, next, inarg->offset + inarg->size);
>  	}
>  	fi->writectr--;
> -	fuse_writepage_finish(fm, wpa);
> +	fuse_writepage_finish(wpa);
>  	spin_unlock(&fi->lock);
>  	fuse_writepage_free(wpa);
>  }

I'm afraid an empty commit message will trigger a warning when running
checkpatch.  Otherwise LGTM.

Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>

-- 
Thanks,
Jingbo

Re: [PATCH 1/2] fuse: drop unused fuse_mount arg in fuse_writepage_finish

[Joanne Koong]

On Mon, Aug 19, 2024 at 6:53 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
>
> On 8/20/24 2:24 AM, Joanne Koong wrote:
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/file.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index f39456c65ed7..63fd5fc6872e 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -1769,8 +1769,7 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa)
> >       kfree(wpa);
> >  }
> >
> > -static void fuse_writepage_finish(struct fuse_mount *fm,
> > -                               struct fuse_writepage_args *wpa)
> > +static void fuse_writepage_finish(struct fuse_writepage_args *wpa)
> >  {
> >       struct fuse_args_pages *ap = &wpa->ia.ap;
> >       struct inode *inode = wpa->inode;
> > @@ -1829,7 +1828,7 @@ __acquires(fi->lock)
> >   out_free:
> >       fi->writectr--;
> >       rb_erase(&wpa->writepages_entry, &fi->writepages);
> > -     fuse_writepage_finish(fm, wpa);
> > +     fuse_writepage_finish(wpa);
> >       spin_unlock(&fi->lock);
> >
> >       /* After fuse_writepage_finish() aux request list is private */
> > @@ -1959,7 +1958,7 @@ static void fuse_writepage_end(struct fuse_mount *fm, struct fuse_args *args,
> >               fuse_send_writepage(fm, next, inarg->offset + inarg->size);
> >       }
> >       fi->writectr--;
> > -     fuse_writepage_finish(fm, wpa);
> > +     fuse_writepage_finish(wpa);
> >       spin_unlock(&fi->lock);
> >       fuse_writepage_free(wpa);
> >  }
>
> I'm afraid an empty commit message will trigger a warning when running
> checkpatch.  Otherwise LGTM.

Gotcha, I'll resubmit this with a commit message. Thanks for taking a look.

>
> Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>
> --
> Thanks,
> Jingbo