Re: [PATCH] nfs/blocklayout: make sure making a aligned read request

[Kinglong Mee <kinglongmee@gmail.com>]

Cc Peng Tao,

On 2/29/2016 21:34, Christoph Hellwig wrote:
> On Mon, Feb 29, 2016 at 08:00:21PM +0800, Kinglong Mee wrote:
>> On 2/29/2016 17:57, Christoph Hellwig wrote:
>>> On Sat, Feb 13, 2016 at 09:51:31PM +0800, Kinglong Mee wrote:
>>>> Only treat write goes up to the inode size as aligned request,
>>>> because it always write PAGE_CACHE_SIZE, but read a dynamic size.
>>>
>>> Can you explain what the point is? 
>>
>> I run ltp tests with read02 hang.
>> There seams a loop in block codes.
>> It is caused by passing an unaligned read to bio.
>> So this patch is out as making a aligned read request.
> 
> Do you have any additional details?

See the following comments.

> 
>>> We'll never use data pas the block size
>>> in the page cache, but per the block size requirement in the spec we must
>>> be able to read it.  This patch means we can't direct storage reads where
>>> we previously could, without any obvious upside.
>>
>> bl_pg_init_read/bl_pg_test_read checks aligned base on SECTOR_SIZE.
>> bl_pg_init_write/bl_pg_test_write checks aligned base on PAGE_SIZE.
>>
>> If according the codes, reads data per block size is okay.
>>
>> But, there is a comment in bl_read_pagelist() as,
>>
>> 250         isect = (sector_t) (f_offset >> SECTOR_SHIFT);
>> 251         /* Code assumes extents are page-aligned */
>> 252         for (i = pg_index; i < header->page_array.npages; i++) {
>> 253                 if (extent_length <= 0) {
>>
>> I don't known the meaning of "extents are page-aligned", 
>> extent's start offset is aligned to page size?
>> or extent's start offset is aligned to page size and length
>> is equal to PAGE_SIZE too ?
> 
> All of them should start aligned to PAGE_SIZE, and also have a length
> that is a multiple of PAGE_SIZE.

Commit f742dc4a3258 ("pnfsblock: fix non-aligned DIO read") has
change the aligned size to SECTOR_SIZE but the request is aligned.

+static bool
+is_aligned_req(struct nfs_page *req, unsigned int alignment)
+{
+	return IS_ALIGNED(req->wb_offset, alignment) &&
+	       IS_ALIGNED(req->wb_bytes, alignment);
+}

Commit 3a6fd1f004fc ("pnfs/blocklayout: remove read-modify-write
 handling in bl_write_pagelist") adds reading up to the inode size.

+       if (req_offset(req) + req->wb_bytes == i_size_read(pgio->pg_inode)) {
+               /*
+                * If the write goes up to the inode size, just write
+                * the full page.  Data past the inode size is
+                * guaranteed to be zeroed by the higher level client
+                * code, and this behaviour is mandated by RFC 5663
+                * section 2.3.2.
+                */
+               return true;
+       }

But the comments are about write request, without any read.
After that patch, the read request can be unaligned.

thanks,
Kinglong Mee