nfs/blocklayout: make sure making a aligned read request

Only treat write goes up to the inode size as aligned request, because it always write PAGE_CACHE_SIZE, but read a dynamic size.

Quoted 2 lines · Kinglong Mee

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?

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.

Quoted 5 lines · Christoph Hellwig

Quoted 2 lines · Kinglong Mee

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.

Quoted 4 lines

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 ?

Quoted 5 lines · Kinglong Mee

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Kinglong Mee

Only treat write goes up to the inode size as aligned request, because it always write PAGE_CACHE_SIZE, but read a dynamic size.

Quoted 6 lines

Quoted 1 line

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?

Quoted 18 lines

Quoted 5 lines

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) {

Quoted 4 lines

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.

Cc Peng Tao,

Quoted 6 lines · Christoph Hellwig

Quoted 5 lines · Kinglong Mee

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Kinglong Mee

Only treat write goes up to the inode size as aligned request, because it always write PAGE_CACHE_SIZE, but read a dynamic size.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 18 lines

Quoted 18 lines

Quoted 5 lines

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) {

Quoted 7 lines

Quoted 4 lines

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.

(quoted diff) +6 -0

+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.

(quoted diff) +10 -0

+       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.

Christoph, does Kinglong's explanation satisfy your concerns, or are there still unresolved differences that should prevent me from taking this patch?