[PATCH RFC] nvmet: fix tmpfs-based backstores support

[PATCH RFC] nvmet: fix tmpfs-based backstores support

[Maurizio Lombardi]

commit 50a909db36f2 ("nvmet: use IOCB_NOWAIT for file-ns buffered I/O")
optimized the read command behaviour  when buffered file-ns
configurations are used.
It issues the read commands with the IOCB_NOWAIT flag set to try
to access the data from the cache.

However, the patch was buggy because both read and write commands are
issued with IOCB_NOWAIT, this broke tmpfs-based backstores because tmpfs
doesn't accept write commands with IOCB_NOWAIT.

Fix this bug by restricting IOCB_NOWAIT to read commands only.

Fixes: 50a909db36f2 ("nvmet: use IOCB_NOWAIT for file-ns buffered I/O")

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/nvme/target/io-cmd-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 6aa30f30b572..1d9b41190583 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -266,6 +266,7 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 
 	if (req->ns->buffered_io) {
 		if (likely(!req->f.mpool_alloc) &&
+				req->cmd->rw.opcode == nvme_cmd_read &&
 				nvmet_file_execute_io(req, IOCB_NOWAIT))
 			return;
 		nvmet_file_submit_buffered_io(req);
-- 
2.27.0

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Chaitanya Kulkarni]

On 11/17/21 07:24, Maurizio Lombardi wrote:
> External email: Use caution opening links or attachments
> 
> 
> commit 50a909db36f2 ("nvmet: use IOCB_NOWAIT for file-ns buffered I/O")
> optimized the read command behaviour  when buffered file-ns

s/behaviour/behavior

> configurations are used.
> It issues the read commands with the IOCB_NOWAIT flag set to try
> to access the data from the cache.
> 
> However, the patch was buggy because both read and write commands are
> issued with IOCB_NOWAIT, this broke tmpfs-based backstores because tmpfs
> doesn't accept write commands with IOCB_NOWAIT.
> 
> Fix this bug by restricting IOCB_NOWAIT to read commands only.
> 
> Fixes: 50a909db36f2 ("nvmet: use IOCB_NOWAIT for file-ns buffered I/O")
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>

We will need blktests for this.

Also, can you share performance numbers with this patch and
the errors that you are getting without this patch ?

> ---
>   drivers/nvme/target/io-cmd-file.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 6aa30f30b572..1d9b41190583 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -266,6 +266,7 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
> 
>          if (req->ns->buffered_io) {
>                  if (likely(!req->f.mpool_alloc) &&
> +                               req->cmd->rw.opcode == nvme_cmd_read &&
>                                  nvmet_file_execute_io(req, IOCB_NOWAIT))
>                          return;
>                  nvmet_file_submit_buffered_io(req);
> --
> 2.27.0
> 

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Maurizio Lombardi]

On Thu, Nov 18, 2021 at 01:59:29AM +0000, Chaitanya Kulkarni wrote:
> We will need blktests for this.
> 
> Also, can you share performance numbers with this patch and
> the errors that you are getting without this patch ?

Ok, I will try to run blktests and provide some performance numbers.

Without this patch, writes get lost:

[root@kvm-02-guest03 ~]# nvme connect -t tcp -a 10.37.153.75 -n test-nqn -s 4080
[root@kvm-02-guest03 ~]# dd if=/dev/urandom of=/dev/nvme0n1 bs=1M count=1
1+0 records in
1+0 records out
1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0604551 s, 17.3 MB/s
[root@kvm-02-guest03 ~]# hexdump -C /dev/nvme0n1 
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
06400000

dmesg output:

[81489.687548] blk_update_request: critical target error, dev nvme0c0n1, sector 0 op 0x1:(WRITE) flags 0x2000800 phys_seg 256 prio class 0
[81489.691139] buffer_io_error: 246 callbacks suppressed
[81489.691141] Buffer I/O error on dev nvme0n1, logical block 0, lost async page write
[81489.695013] Buffer I/O error on dev nvme0n1, logical block 1, lost async page write
[81489.697183] Buffer I/O error on dev nvme0n1, logical block 2, lost async page write
[81489.699239] Buffer I/O error on dev nvme0n1, logical block 3, lost async page write
[81489.701297] Buffer I/O error on dev nvme0n1, logical block 4, lost async page write
[81489.703374] Buffer I/O error on dev nvme0n1, logical block 5, lost async page write
[81489.705459] Buffer I/O error on dev nvme0n1, logical block 6, lost async page write
[81489.707511] Buffer I/O error on dev nvme0n1, logical block 7, lost async page write
[81489.709438] Buffer I/O error on dev nvme0n1, logical block 8, lost async page write
[81489.711364] Buffer I/O error on dev nvme0n1, logical block 9, lost async page write

Maurizio

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Chaitanya Kulkarni]

On 11/18/2021 12:29 AM, Maurizio Lombardi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Nov 18, 2021 at 01:59:29AM +0000, Chaitanya Kulkarni wrote:
>> We will need blktests for this.
>>
>> Also, can you share performance numbers with this patch and
>> the errors that you are getting without this patch ?
> 
> Ok, I will try to run blktests and provide some performance numbers.
> 
> Without this patch, writes get lost:
> 
> [root@kvm-02-guest03 ~]# nvme connect -t tcp -a 10.37.153.75 -n test-nqn -s 4080
> [root@kvm-02-guest03 ~]# dd if=/dev/urandom of=/dev/nvme0n1 bs=1M count=1
> 1+0 records in
> 1+0 records out
> 1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0604551 s, 17.3 MB/s
> [root@kvm-02-guest03 ~]# hexdump -C /dev/nvme0n1
> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 06400000
> 
> dmesg output:
> 
> [81489.687548] blk_update_request: critical target error, dev nvme0c0n1, sector 0 op 0x1:(WRITE) flags 0x2000800 phys_seg 256 prio class 0
> [81489.691139] buffer_io_error: 246 callbacks suppressed
> [81489.691141] Buffer I/O error on dev nvme0n1, logical block 0, lost async page write
> [81489.695013] Buffer I/O error on dev nvme0n1, logical block 1, lost async page write
> [81489.697183] Buffer I/O error on dev nvme0n1, logical block 2, lost async page write
> [81489.699239] Buffer I/O error on dev nvme0n1, logical block 3, lost async page write
> [81489.701297] Buffer I/O error on dev nvme0n1, logical block 4, lost async page write
> [81489.703374] Buffer I/O error on dev nvme0n1, logical block 5, lost async page write
> [81489.705459] Buffer I/O error on dev nvme0n1, logical block 6, lost async page write
> [81489.707511] Buffer I/O error on dev nvme0n1, logical block 7, lost async page write
> [81489.709438] Buffer I/O error on dev nvme0n1, logical block 8, lost async page write
> [81489.711364] Buffer I/O error on dev nvme0n1, logical block 9, lost async page write
> 
> Maurizio
> 

Thanks for the prompt reply and sharing this ..

Please send a formal-patch (non-RFC) with the testing done.

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Chaitanya Kulkarni]

On 11/18/2021 12:29 AM, Maurizio Lombardi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Nov 18, 2021 at 01:59:29AM +0000, Chaitanya Kulkarni wrote:
>> We will need blktests for this.
>>


One more thing you need to add a new block tests with the
configuration that you have used to find this problem so it
will get tested every release.

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Maurizio Lombardi]

On Thu, Nov 18, 2021 at 08:42:25AM +0000, Chaitanya Kulkarni wrote:
> One more thing you need to add a new block tests with the
> configuration that you have used to find this problem so it
> will get tested every release.
>

Ok, will do.

Maurizio

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Christoph Hellwig]

The proper fix is to check if the file has FMODE_NOWAIT is set before
using FMODE_NOWAIT for reads or writes, not to blindly disable it for
reads.

Re: [PATCH RFC] nvmet: fix tmpfs-based backstores support

[Maurizio Lombardi]

On Fri, Nov 19, 2021 at 06:02:33PM +0100, Christoph Hellwig wrote:
> The proper fix is to check if the file has FMODE_NOWAIT is set before
> using FMODE_NOWAIT for reads or writes, not to blindly disable it for
> reads.
>

Ah thanks for the info!
Now I see that filesystems that support NOWAIT set the FMODE_NOWAIT
flag in their open() functions.

So the correct patch should be the following:

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 1dd1a0fe2e81..bf3f440c98d5 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -266,6 +266,7 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 
        if (req->ns->buffered_io) {
                if (likely(!req->f.mpool_alloc) &&
+                               (req->ns->file->f_mode & FMODE_NOWAIT) &&
                                nvmet_file_execute_io(req, IOCB_NOWAIT))
                        return;
                nvmet_file_submit_buffered_io(req);


I'm going to test it.

Maurizio