Re: [PATCH 3/3] iomap: optimize iomap_is_partially_uptodate for full page range

[Eric Sandeen <sandeen@sandeen.net>]

On 12/13/18 9:05 PM, Dave Chinner wrote:
> On Thu, Dec 13, 2018 at 04:25:29PM -0600, Eric Sandeen wrote:
>> From: Eric Sandeen <sandeen@redhat.com>
>>
>> We only call ->is_partially_uptodate to look for an uptodate range
>> within a not-uptodate page.
>>
>> If the range covers all blocks in the page, there is no point to checking
>> each block individually - if the whole range (i.e. the whole page) were
>> uptodate, the page would be uptodate as well.  Hence in this case, we
>> can return early and skip the loop.
>>
>> This is similar to what is done in block_is_partially_uptodate().
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>  fs/iomap.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/iomap.c b/fs/iomap.c
>> index ce837d9..7d7d985 100644
>> --- a/fs/iomap.c
>> +++ b/fs/iomap.c
>> @@ -515,6 +515,10 @@ struct iomap_readpage_ctx {
>>  	first = from >> inode->i_blkbits;
>>  	last = (from + len - 1) >> inode->i_blkbits;
>>  
>> +	/* If page wasn't uptodate and range covers all blocks: no partial */
>> +	if (first == 0 && last == (PAGE_SIZE - 1) >> inode->i_blkbits)
>> +		return 0;
>> +
> 
> Even if the range covers the entire page, we still have to check
> that all the individual blocks in the page are up to date or not.
> Hence I think this is wrong....

Didn't PageUptodate(page) answer that question already in the caller?

The caller does:

                        if (PageUptodate(page))
                                goto page_ok;
...
                        if (!mapping->a_ops->is_partially_uptodate(page,
                                                        offset, iter->count))
                                goto page_not_up_to_date_locked;


So if it was found to be uptodate, we never called this op.  If we /were/
called (page was not uptodate) that can only be because at least one block in
the page is not uptodate.  So what's the need for checking them all again?

(unless it's because we're now doing it under the page lock?)

block_is_partially_uptodate does the same thing:

        if (from < blocksize && to > PAGE_SIZE - blocksize)
                return 0;

Still, this also seems like maybe something the caller should do instead.

-Eric