As the block and SCSI layouts can only read/write fixed-length
blocks, we must perform read-modify-write when data to be written is
not aligned to a block boundary or smaller than the block size.
(612aa983a0410 pnfs: add flag to force read-modify-write in ->write_begin)
The current code tries to see if we have to do read-modify-write
on block-oriented pNFS layouts by just checking !PageUptodate(page),
but the same condition also applies for overwriting of any uncached
potions of existing files, making such operations excessively slow
even if it is block-aligned.
The change does not affect the optimization for modify-write-read
cases (38c73044f5f4d NFS: read-modify-write page updating),
because partial update of !PageUptodate() pages can only happen
in layouts that can do arbitrary length read/write and never
in block-based ones.
Testing results:
We ran fio on one of the pNFS clients running 4.20 kernel
(vanilla and patched) in this configuration to read/write/overwrite
files on the storage array, exported as pnfs share by the server.
pNFS clients ---1G Ethernet--- pNFS server
(HP DL360 G8) (HP DL360 G8)
| |
| |
+------8G Fiber Channel--------+
|
Storage Array
(HP P6350)
Throughput of overwrite (both buffered and O_SYNC) is noticeably
improved.
Ops. |block size| Throughput |
| (KiB) | (MiB/s) |
| | 4.20 | patched|
---------+----------+----------------+
buffered | 4| 21.3 | 233 |
overwrite| 32| 22.2 | 254 |
| 512| 22.4 | 258 |
---------+----------+----------------+
O_SYNC | 4| 3.84| 4.68|
overwrite| 32| 12.2 | 32.9 |
| 512| 18.5 | 151 |
---------+----------+----------------+
Read and write (buffered and O_SYNC) by the same client remain unchanged
by the patch either negatively or positively, as they should do.
Ops. |block size| Throughput |
| (KiB) | (MiB/s) |
| | 4.20 | patched|
---------+----------+----------------+
read | 4| 548 | 551 |
| 32| 547 | 552 |
| 512| 548 | 549 |
---------+----------+----------------+
buffered | 4| 237 | 243 |
write | 32| 261 | 267 |
| 512| 265 | 271 |
---------+----------+----------------+
O_SYNC | 4| 0.46| 0.46|
write | 32| 3.60| 3.61|
| 512| 105 | 108 |
---------+----------+----------------+
Signed-off-by: Kazuo Ito <[email protected]>
Tested-by: Hiroyuki Watabane <[email protected]>
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 29553fdba8af..e80954c96ec1 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -276,6 +276,12 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
* then a modify/write/read cycle when writing to a page in the
* page cache.
*
+ * Some pNFS layout drivers can only read/write at a certain block
+ * granularity like all block devices and therefore we must perform
+ * read/modify/write whenever a page hasn't read yet and the data
+ * to be written there is not aligned to a block boundary and/or
+ * smaller than the block size.
+ *
* The modify/write/read cycle may occur if a page is read before
* being completely filled by the writer. In this situation, the
* page must be completely written to stable storage on the server
@@ -299,8 +305,10 @@ static int nfs_want_read_modify_write(struct file
*file, struct page *page,
unsigned int end = offset + len;
if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
- if (!PageUptodate(page))
- return 1;
+ if (!PageUptodate(page)) {
+ if (pglen && (end < pglen || offset))
+ return 1;
+ }
return 0;
}
On 7 Feb 2019, at 3:12, Kazuo Ito wrote:
> As the block and SCSI layouts can only read/write fixed-length
> blocks, we must perform read-modify-write when data to be written is
> not aligned to a block boundary or smaller than the block size.
> (612aa983a0410 pnfs: add flag to force read-modify-write in
> ->write_begin)
>
> The current code tries to see if we have to do read-modify-write
> on block-oriented pNFS layouts by just checking !PageUptodate(page),
> but the same condition also applies for overwriting of any uncached
> potions of existing files, making such operations excessively slow
> even if it is block-aligned.
>
> The change does not affect the optimization for modify-write-read
> cases (38c73044f5f4d NFS: read-modify-write page updating),
> because partial update of !PageUptodate() pages can only happen
> in layouts that can do arbitrary length read/write and never
> in block-based ones.
>
> Testing results:
>
> We ran fio on one of the pNFS clients running 4.20 kernel
> (vanilla and patched) in this configuration to read/write/overwrite
> files on the storage array, exported as pnfs share by the server.
>
> pNFS clients ---1G Ethernet--- pNFS server
> (HP DL360 G8) (HP DL360 G8)
> | |
> | |
> +------8G Fiber Channel--------+
> |
> Storage Array
> (HP P6350)
>
> Throughput of overwrite (both buffered and O_SYNC) is noticeably
> improved.
>
> Ops. |block size| Throughput |
> | (KiB) | (MiB/s) |
> | | 4.20 | patched|
> ---------+----------+----------------+
> buffered | 4| 21.3 | 233 |
> overwrite| 32| 22.2 | 254 |
> | 512| 22.4 | 258 |
> ---------+----------+----------------+
> O_SYNC | 4| 3.84| 4.68|
> overwrite| 32| 12.2 | 32.9 |
> | 512| 18.5 | 151 |
> ---------+----------+----------------+
>
> Read and write (buffered and O_SYNC) by the same client remain
> unchanged
> by the patch either negatively or positively, as they should do.
>
> Ops. |block size| Throughput |
> | (KiB) | (MiB/s) |
> | | 4.20 | patched|
> ---------+----------+----------------+
> read | 4| 548 | 551 |
> | 32| 547 | 552 |
> | 512| 548 | 549 |
> ---------+----------+----------------+
> buffered | 4| 237 | 243 |
> write | 32| 261 | 267 |
> | 512| 265 | 271 |
> ---------+----------+----------------+
> O_SYNC | 4| 0.46| 0.46|
> write | 32| 3.60| 3.61|
> | 512| 105 | 108 |
> ---------+----------+----------------+
>
> Signed-off-by: Kazuo Ito <[email protected]>
> Tested-by: Hiroyuki Watabane <[email protected]>
>
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 29553fdba8af..e80954c96ec1 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -276,6 +276,12 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
> * then a modify/write/read cycle when writing to a page in the
> * page cache.
> *
> + * Some pNFS layout drivers can only read/write at a certain block
> + * granularity like all block devices and therefore we must perform
> + * read/modify/write whenever a page hasn't read yet and the data
> + * to be written there is not aligned to a block boundary and/or
> + * smaller than the block size.
> + *
> * The modify/write/read cycle may occur if a page is read before
> * being completely filled by the writer. In this situation, the
> * page must be completely written to stable storage on the server
> @@ -299,8 +305,10 @@ static int nfs_want_read_modify_write(struct file
> *file, struct page *page,
> unsigned int end = offset + len;
>
> if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
> - if (!PageUptodate(page))
> - return 1;
> + if (!PageUptodate(page)) {
> + if (pglen && (end < pglen || offset))
> + return 1;
> + }
> return 0;
> }
This looks right. I think that a static inline bool
nfs_write_covers_page,
or full_page_write or similar might make sense here, as we do the same
test
just below, and would make the code easier to quickly understand.
Reviewed-by: Benjamin Coddington <[email protected]>
Ben
On 2019/02/07 22:37, Benjamin Coddington wrote:
> On 7 Feb 2019, at 3:12, Kazuo Ito wrote:
> [snipped]
>> @@ -299,8 +305,10 @@ static int nfs_want_read_modify_write(struct file
>> *file, struct page *page,
>> unsigned int end = offset + len;
>>
>> if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
>> - if (!PageUptodate(page))
>> - return 1;
>> + if (!PageUptodate(page)) {
>> + if (pglen && (end < pglen || offset))
>> + return 1;
>> + }
>> return 0;
>> }
>
> This looks right. I think that a static inline bool nfs_write_covers_page,
> or full_page_write or similar might make sense here, as we do the same test
> just below, and would make the code easier to quickly understand.
>
> Reviewed-by: Benjamin Coddington <[email protected]>
>
> Ben
As per Ben's comment, I made the check for full page write a static
inline function and both the block-oriented and the non-block-
oriented paths call it.
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 29553fdba8af..458c77ccf274 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -276,6 +276,12 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
* then a modify/write/read cycle when writing to a page in the
* page cache.
*
+ * Some pNFS layout drivers can only read/write at a certain block
+ * granularity like all block devices and therefore we must perform
+ * read/modify/write whenever a page hasn't read yet and the data
+ * to be written there is not aligned to a block boundary and/or
+ * smaller than the block size.
+ *
* The modify/write/read cycle may occur if a page is read before
* being completely filled by the writer. In this situation, the
* page must be completely written to stable storage on the server
@@ -291,15 +297,23 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
* and that the new data won't completely replace the old data in
* that range of the file.
*/
-static int nfs_want_read_modify_write(struct file *file, struct page *page,
- loff_t pos, unsigned len)
+static bool nfs_full_page_write(struct page *page, loff_t pos, unsigned
len)
{
unsigned int pglen = nfs_page_length(page);
unsigned int offset = pos & (PAGE_SIZE - 1);
unsigned int end = offset + len;
+ if (pglen && ((end < pglen) || offset))
+ return 0;
+ return 1;
+}
+
+static int nfs_want_read_modify_write(struct file *file, struct page *page,
+ loff_t pos, unsigned len)
+{
if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
- if (!PageUptodate(page))
+ if (!PageUptodate(page) &&
+ !nfs_full_page_write(page, pos, len))
return 1;
return 0;
}
@@ -307,8 +321,7 @@ static int nfs_want_read_modify_write(struct file
*file, struct page *page,
if ((file->f_mode & FMODE_READ) && /* open for read? */
!PageUptodate(page) && /* Uptodate? */
!PagePrivate(page) && /* i/o request already? */
- pglen && /* valid bytes of file? */
- (end < pglen || offset)) /* replace all valid bytes? */
+ !nfs_full_page_write(page, pos, len))
return 1;
return 0;
}
Signed-off-by: Kazuo Ito <[email protected]>
On Fri, 2019-02-08 at 16:54 +0900, 伊藤和夫 wrote:
> On 2019/02/07 22:37, Benjamin Coddington wrote:
> > On 7 Feb 2019, at 3:12, Kazuo Ito wrote:
> > [snipped]
> > > @@ -299,8 +305,10 @@ static int nfs_want_read_modify_write(struct
> > > file
> > > *file, struct page *page,
> > > unsigned int end = offset + len;
> > >
> > > if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
> > > - if (!PageUptodate(page))
> > > - return 1;
> > > + if (!PageUptodate(page)) {
> > > + if (pglen && (end < pglen || offset))
> > > + return 1;
> > > + }
> > > return 0;
> > > }
> >
> > This looks right. I think that a static inline bool
> > nfs_write_covers_page,
> > or full_page_write or similar might make sense here, as we do the
> > same test
> > just below, and would make the code easier to quickly understand.
> >
> > Reviewed-by: Benjamin Coddington <[email protected]>
> >
> > Ben
>
> As per Ben's comment, I made the check for full page write a static
> inline function and both the block-oriented and the non-block-
> oriented paths call it.
>
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 29553fdba8af..458c77ccf274 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -276,6 +276,12 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
> * then a modify/write/read cycle when writing to a page in the
> * page cache.
> *
> + * Some pNFS layout drivers can only read/write at a certain block
> + * granularity like all block devices and therefore we must perform
> + * read/modify/write whenever a page hasn't read yet and the data
> + * to be written there is not aligned to a block boundary and/or
> + * smaller than the block size.
> + *
> * The modify/write/read cycle may occur if a page is read before
> * being completely filled by the writer. In this situation, the
> * page must be completely written to stable storage on the server
> @@ -291,15 +297,23 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
> * and that the new data won't completely replace the old data in
> * that range of the file.
> */
> -static int nfs_want_read_modify_write(struct file *file, struct page
> *page,
> - loff_t pos, unsigned len)
> +static bool nfs_full_page_write(struct page *page, loff_t pos,
> unsigned
> len)
> {
> unsigned int pglen = nfs_page_length(page);
> unsigned int offset = pos & (PAGE_SIZE - 1);
> unsigned int end = offset + len;
>
> + if (pglen && ((end < pglen) || offset))
> + return 0;
> + return 1;
> +}
> +
> +static int nfs_want_read_modify_write(struct file *file, struct page
> *page,
> + loff_t pos, unsigned len)
> +{
> if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
> - if (!PageUptodate(page))
> + if (!PageUptodate(page) &&
> + !nfs_full_page_write(page, pos, len))
> return 1;
> return 0;
> }
> @@ -307,8 +321,7 @@ static int nfs_want_read_modify_write(struct
> file
> *file, struct page *page,
> if ((file->f_mode & FMODE_READ) && /* open for read? */
> !PageUptodate(page) && /* Uptodate? */
> !PagePrivate(page) && /* i/o request already? */
> - pglen && /* valid bytes of
> file? */
> - (end < pglen || offset)) /* replace all valid
> bytes? */
> + !nfs_full_page_write(page, pos, len))
> return 1;
> return 0;
> }
How about adding a separate
if (PageUptodate(page) || nfs_full_page_write())
return 0;
before the check for pNFS?
That means we won't have to duplicate those for the pNFS block and
ordinary case, and it improves code clarity.
BTW: Why doesn't the pNFS case check for PagePrivate(page)? That looks
like a bug which would cause the existing write to get corrupted.
If so, we should move that check too into the common code.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On 2019/02/08 23:58, Trond Myklebust wrote:
> On Fri, 2019-02-08 at 16:54 +0900, 伊藤和夫 wrote:
>> On 2019/02/07 22:37, Benjamin Coddington wrote:
>>> On 7 Feb 2019, at 3:12, Kazuo Ito wrote:
>>> [snipped]
>>>> @@ -299,8 +305,10 @@ static int nfs_want_read_modify_write(struct
>>>> file
>>>> *file, struct page *page,
>>>> unsigned int end = offset + len;
>>>>
>>>> if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
>>>> - if (!PageUptodate(page))
>>>> - return 1;
>>>> + if (!PageUptodate(page)) {
>>>> + if (pglen && (end < pglen || offset))
>>>> + return 1;
>>>> + }
>>>> return 0;
>>>> }
>>>
>>> This looks right. I think that a static inline bool
>>> nfs_write_covers_page,
>>> or full_page_write or similar might make sense here, as we do the
>>> same test
>>> just below, and would make the code easier to quickly understand.
>>>
>>> Reviewed-by: Benjamin Coddington <[email protected]>
>> >
>> > Ben
>>
>> As per Ben's comment, I made the check for full page write a static
>> inline function and both the block-oriented and the non-block-
>> oriented paths call it.
>>
>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>> index 29553fdba8af..458c77ccf274 100644
>> --- a/fs/nfs/file.c
>> +++ b/fs/nfs/file.c
>> @@ -276,6 +276,12 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
>> * then a modify/write/read cycle when writing to a page in the
>> * page cache.
>> *
>> + * Some pNFS layout drivers can only read/write at a certain block
>> + * granularity like all block devices and therefore we must perform
>> + * read/modify/write whenever a page hasn't read yet and the data
>> + * to be written there is not aligned to a block boundary and/or
>> + * smaller than the block size.
>> + *
>> * The modify/write/read cycle may occur if a page is read before
>> * being completely filled by the writer. In this situation, the
>> * page must be completely written to stable storage on the server
>> @@ -291,15 +297,23 @@ EXPORT_SYMBOL_GPL(nfs_file_fsync);
>> * and that the new data won't completely replace the old data in
>> * that range of the file.
>> */
>> -static int nfs_want_read_modify_write(struct file *file, struct page
>> *page,
>> - loff_t pos, unsigned len)
>> +static bool nfs_full_page_write(struct page *page, loff_t pos,
>> unsigned
>> len)
>> {
>> unsigned int pglen = nfs_page_length(page);
>> unsigned int offset = pos & (PAGE_SIZE - 1);
>> unsigned int end = offset + len;
>>
>> + if (pglen && ((end < pglen) || offset))
>> + return 0;
>> + return 1;
>> +}
>> +
>> +static int nfs_want_read_modify_write(struct file *file, struct page
>> *page,
>> + loff_t pos, unsigned len)
>> +{
>> if (pnfs_ld_read_whole_page(file->f_mapping->host)) {
>> - if (!PageUptodate(page))
>> + if (!PageUptodate(page) &&
>> + !nfs_full_page_write(page, pos, len))
>> return 1;
>> return 0;
>> }
>> @@ -307,8 +321,7 @@ static int nfs_want_read_modify_write(struct
>> file
>> *file, struct page *page,
>> if ((file->f_mode & FMODE_READ) && /* open for read? */
>> !PageUptodate(page) && /* Uptodate? */
>> !PagePrivate(page) && /* i/o request already? */
>> - pglen && /* valid bytes of
>> file? */
>> - (end < pglen || offset)) /* replace all valid
>> bytes? */
>> + !nfs_full_page_write(page, pos, len))
>> return 1;
>> return 0;
>> }
>
> How about adding a separate
>
> if (PageUptodate(page) || nfs_full_page_write())
> return 0;
>
> before the check for pNFS?
>
> That means we won't have to duplicate those for the pNFS block and
> ordinary case, and it improves code clarity.
Yes, it is much better, and
> BTW: Why doesn't the pNFS case check for PagePrivate(page)? That looks
> like a bug which would cause the existing write to get corrupted.
> If so, we should move that check too into the common code.
It's been that way since the check for
pnfs_ld_read_whole_page(file->f_mapping->host) was added there.
As you pointed out, it shouldn't try to initiate read when there's
an outstanding write.
So, I'll update the patch with these changes, including check for
ongoing I/O, and come up with newer test results in a couple of days.
--
kazuo ito ([email protected])
NTT OSS Center