On Wed, Nov 28, 2018 at 02:40:07PM +0000, Filipe Manana wrote: > > > Well, the worker tasks can also not use gfp_kernel, since the scrub > > > task waits for them to complete before pausing. > > > I missed this, and 2 reviewers as well, so perhaps it wasn't that > > > trivial and I shouldn't feel that I miserably failed to identify all > > > cases for something rather trivial. V5 sent. > > > > You can say that you left it there intentionally, such cookies are a > > good drill for reviewers to stay sharp. > > Unfortunately for me, it was not on purpose. > > However there's the small drill of, for the workers only, potentially > moving the memalloc_nofs_save() and > restore to scrub_handle_errored_block(), since the only two possible > gfp_kernel allocations for workers > are during the case where a repair is needed: > > 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 > > Just to avoid some duplication.
Sounds like a nice cleanup and more in line with the idea of scoped GFP_NOFS, the markers should be at a higher level. Starting at the bottom of the callstack is fine as it's obvious where we want the nofs protection, and then push it up the call chain. That way it's easier to review. Please send a followup patch, thanks.
