Re: RFA (Request for Advice): block/bio: get_user_pages() --> pin_user_pages()

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 1/24/22 2:05 AM, Jan Kara wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hello,
> 
> On Sun 23-01-22 23:52:07, John Hubbard wrote:
>> Background: despite having very little experience in the block and bio
>> layers, I am attempting to convert the Direct IO parts of them from
>> using get_user_pages_fast(), to pin_user_pages_fast(). This requires the
>> use of a corresponding special release call: unpin_user_pages(), instead
>> of the generic put_page().
>>
>> Fortunately, Christoph Hellwig has observed [1] (more than once [2]) that
>> only "a little" refactoring is required, because it is *almost* true
>> that bio_release_pages() could just be switched over from calling
>> put_page(), to unpin_user_page(). The "not quite" part is mainly due to
>> the zero page. There are a few write paths that pad zeroes, and they use
>> the zero page.
>>
>> That's where I'd like some advice. How to refactor things, so that the
>> zero page does not end up in the list of pages that bio_release_pages()
>> acts upon?

this maybe wrong but thinking out loudly, have you consider adding a 
ZERO_PAGE() address check since it should have a unique same
address for each ZERO_PAGE() (unless I'm totally wrong here) and
using this check you can distinguish between ZERO_PAGE() and
non ZERO_PAGE() on the bio list in bio_release_pages().

>>
>> To ground this in reality, one of the partial call stacks is:
>>
>> do_direct_IO()
>>      dio_zero_block()
>>          page = ZERO_PAGE(0); <-- This is a problem
>>
>> I'm not sure what to use, instead of that zero page! The zero page
>> doesn't need to be allocated nor tracked, and so any replacement
>> approaches would need either other storage, or some horrid scheme that I
>> won't go so far as to write on the screen. :)
> 
> Well, I'm not sure if you consider this ugly but currently we use
> get_page() in that path exactly so that bio_release_pages() does not have
> to care about zero page. So now we could grab pin on the zero page instead
> through try_grab_page() or something like that...
> 
>        

submit_page_section() does call get_page() in that same path 
irrespective of whether it is ZERO_PAGE() or not, this actually
makes accounting much easier and we also avoid  any special case
for ZERO_PAGE().

dio_zero_block()
  submit_page_section()
    get_page()


That also means that on completion of dio for each bio we also call
put_page() from bio_release_page() path.

                                                           Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>