Re: [PATCH 2/2] FUSE: fix congested state leak on aborted connections

[Miklos Szeredi]

On Tue, Feb 6, 2018 at 5:25 PM, Jan Kara  wrote:
> On Fri 02-02-18 09:54:14, Tejun Heo wrote:
>> If a connection gets aborted while congested, FUSE can leave
>> nr_wb_congested[] stuck until reboot causing wait_iff_congested() to
>> wait spuriously which can lead to severe performance degradation.
>>
>> The leak is caused by gating congestion state clearing with
>> fc->connected test in request_end().  This was added way back in 2009
>> by 26c3679101db ("fuse: destroy bdi on umount").  While the commit
>> description doesn't explain why the test was added, it most likely was
>> to avoid dereferencing bdi after it got destroyed.
>>
>> Since then, bdi lifetime rules have changed many times and now we're
>> always guaranteed to have access to the bdi while the superblock is
>> alive (fc->sb).
>>
>> Drop fc->connected conditional to avoid leaking congestion states.
>>
>> Signed-off-by: Tejun Heo 
>> Reported-by: Joshua Miller 
>> Cc: Johannes Weiner 
>> Cc: Miklos Szeredi 
>> Cc: Jan Kara 
>> Cc: sta...@vger.kernel.org
>
> Yeah, this should be fine AFAICT but my knowledge of FUSE is very cursory.
> Anyway:
>
> Acked-by: Jan Kara 

Can't say I fully understand how the global "is any bdi congested"
state is used in direct reclaim, but the patch is an obvious
improvement, so applied.

Thanks,
Miklos

Re: [PATCH 2/2] FUSE: fix congested state leak on aborted connections

[Jan Kara]

On Fri 02-02-18 09:54:14, Tejun Heo wrote:
> If a connection gets aborted while congested, FUSE can leave
> nr_wb_congested[] stuck until reboot causing wait_iff_congested() to
> wait spuriously which can lead to severe performance degradation.
> 
> The leak is caused by gating congestion state clearing with
> fc->connected test in request_end().  This was added way back in 2009
> by 26c3679101db ("fuse: destroy bdi on umount").  While the commit
> description doesn't explain why the test was added, it most likely was
> to avoid dereferencing bdi after it got destroyed.
> 
> Since then, bdi lifetime rules have changed many times and now we're
> always guaranteed to have access to the bdi while the superblock is
> alive (fc->sb).
> 
> Drop fc->connected conditional to avoid leaking congestion states.
> 
> Signed-off-by: Tejun Heo 
> Reported-by: Joshua Miller 
> Cc: Johannes Weiner 
> Cc: Miklos Szeredi 
> Cc: Jan Kara 
> Cc: sta...@vger.kernel.org

Yeah, this should be fine AFAICT but my knowledge of FUSE is very cursory.
Anyway:

Acked-by: Jan Kara 

Honza

> ---
>  fs/fuse/dev.c |3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -381,8 +381,7 @@ static void request_end(struct fuse_conn
>   if (!fc->blocked && waitqueue_active(&fc->blocked_waitq))
>   wake_up(&fc->blocked_waitq);
>  
> - if (fc->num_background == fc->congestion_threshold &&
> - fc->connected && fc->sb) {
> + if (fc->num_background == fc->congestion_threshold && fc->sb) {
>   clear_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>   clear_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>   }
-- 
Jan Kara 
SUSE Labs, CR

[PATCH 2/2] FUSE: fix congested state leak on aborted connections

[Tejun Heo]

If a connection gets aborted while congested, FUSE can leave
nr_wb_congested[] stuck until reboot causing wait_iff_congested() to
wait spuriously which can lead to severe performance degradation.

The leak is caused by gating congestion state clearing with
fc->connected test in request_end().  This was added way back in 2009
by 26c3679101db ("fuse: destroy bdi on umount").  While the commit
description doesn't explain why the test was added, it most likely was
to avoid dereferencing bdi after it got destroyed.

Since then, bdi lifetime rules have changed many times and now we're
always guaranteed to have access to the bdi while the superblock is
alive (fc->sb).

Drop fc->connected conditional to avoid leaking congestion states.

Signed-off-by: Tejun Heo 
Reported-by: Joshua Miller 
Cc: Johannes Weiner 
Cc: Miklos Szeredi 
Cc: Jan Kara 
Cc: sta...@vger.kernel.org
---
 fs/fuse/dev.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -381,8 +381,7 @@ static void request_end(struct fuse_conn
if (!fc->blocked && waitqueue_active(&fc->blocked_waitq))
wake_up(&fc->blocked_waitq);
 
-   if (fc->num_background == fc->congestion_threshold &&
-   fc->connected && fc->sb) {
+   if (fc->num_background == fc->congestion_threshold && fc->sb) {
clear_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
clear_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
}