Re: [PATCH] Btrfs: scrub, move setup of nofs contexts higher in the stack

[David Sterba <dsterba@suse.cz>]

On Fri, Dec 07, 2018 at 01:23:32PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Since scrub workers only do memory allocation with GFP_KERNEL when they
> need to perform repair, we can move the recent setup of the nofs context
> up to scrub_handle_errored_block() instead of setting it up down the call
> chain at insert_full_stripe_lock() and scrub_add_page_to_wr_bio(),
> removing some duplicate code and comment. So the only paths for which a
> scrub worker can do memory allocations using GFP_KERNEL are the following:
> 
>  scrub_bio_end_io_worker()
>    scrub_block_complete()
>      scrub_handle_errored_block()
>        lock_full_stripe()
>          insert_full_stripe_lock()
>            -> kmalloc with GFP_KERNEL
> 
>   scrub_bio_end_io_worker()
>     scrub_block_complete()
>       scrub_handle_errored_block()
>         scrub_write_page_to_dev_replace()
>           scrub_add_page_to_wr_bio()
>             -> kzalloc with GFP_KERNEL
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.

Reviewed-by: David Sterba <dsterba@suse.com>

> @@ -867,6 +860,16 @@ static int scrub_handle_errored_block(struct scrub_block *sblock_to_check)

> +	nofs_flag = memalloc_nofs_save();

>  	ret = lock_full_stripe(fs_info, logical, &full_stripe_locked);

>  	ret = unlock_full_stripe(fs_info, logical, full_stripe_locked);
> +	memalloc_nofs_restore(nofs_flag);

Though I don't see a technical reason for placing the restore after
unlock_full_stripe (no GFP_KERNEL allocations there and not in the
preceding block that does only kfree) it looks better to keep the proper
nesting of memalloc save/lock/unlock/memalloc restore. So agreed, just
that it's a new pattern I think it's good to spell it out.