| Message ID | 20240819182417.504672-2-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] fuse: drop unused fuse_mount arg in fuse_writepage_finish | expand |
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().
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
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
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
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
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
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); }
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(-)