cifs conversion to netfslib

cifs conversion to netfslib

[David Howells]

Hi Rohith,

Take a look at the patches at the top of my cifs-experimental branch.  I put
in a ->clamp_length() method and used that to chop up read requests into
subrequests according to credits and rsize rather than trying to limit the
size of a read in ->issue_read().  It behaves a lot better now.  I see the VM
creating 8M requests that then get broken up into pairs of 4M read RPCs that
then take place in parallel.

I've taken the idea of allowing the netfs to propose larger allocations for
the request and subrequest structs and, in effect, merged the cifs_readdata
struct with the netfs_io_subrequest struct reducing the amount of overhead a
bit.  I moved the cifsFileInfo field out from the subrequest to a wrapper for
the request struct, so that it's carried for all the subreqs in common.

Possibly some other readdata fields could be eliminated too as being
superfluous to the fields in the netfs_io_subrequest struct.  offset,
got_bytes, bytes and result for example.

There are a couple of problems with splice write, though, at least one of
which is probably due to the iteratorisation.  Firstly, xfstests now gets as
far as generic/013 and then gets to a point where one of the threads is just
sitting there sending the same splice write over and over again and getting a
zero result back.  Running splice by hand, however, shows no problem and
because it's in fsstress, I think, it's really hard to work out how it gets to
this point:-/.

The other issue is that if I run splice to an empty file, it works; running
another splice to the same file will result in the server giving
STATUS_ACCESS_DENIED when cifs_write_begin() tries to read from the file:

    7 0.009485249  192.168.6.2 → 192.168.6.1  SMB2 183 Read Request Len:65536 Off:0 File: x
    8 0.009674245  192.168.6.1 → 192.168.6.2  SMB2 143 Read Response, Error: STATUS_ACCESS_DENIED

Actually - that might be because the file is only 65536 bytes long because the
first splice finished short.

David

Re: cifs conversion to netfslib

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> The other issue is that if I run splice to an empty file, it works; running
> another splice to the same file will result in the server giving
> STATUS_ACCESS_DENIED when cifs_write_begin() tries to read from the file:
> 
>     7 0.009485249  192.168.6.2 → 192.168.6.1  SMB2 183 Read Request Len:65536 Off:0 File: x
>     8 0.009674245  192.168.6.1 → 192.168.6.2  SMB2 143 Read Response, Error: STATUS_ACCESS_DENIED
> 
> Actually - that might be because the file is only 65536 bytes long because the
> first splice finished short.

Actually, it's because I opened the output file O_WRONLY.  If I open it
O_RDWR, it works.  The test program is attached below.

David
---
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>

int main(int argc, char *argv[])
{
	off64_t opos;
	size_t len;
	int in, out;

	if (argc != 4) {
		printf("Format: %s size in out\n", argv[0]);
		exit(2);
	}

	len = atol(argv[1]);

	if (strcmp(argv[2], "-") != 0) {
		in = open(argv[2], O_RDONLY);
		if (in < 0) {
			perror(argv[2]);
			return 1;
		}
	} else {
		in = 0;
	}

	if (strcmp(argv[3], "-") != 0) {
		out = open(argv[3], O_WRONLY);  // Change to O_RDWR
		if (out < 0) {
			perror(argv[3]);
			return 1;
		}
	} else {
		out = 1;
	}

	opos = 3;
	if (splice(in, NULL, out, &opos, len, 0) < 0) {
		perror("splice");
		return 1;
	}

	if (close(in) < 0) {
		perror("close/in");
		return 1;
	}

	if (close(out) < 0) {
		perror("close/out");
		return 1;
	}

	return 0;
}

Re: cifs conversion to netfslib

[Rohith Surabattula]

Hi David,

Below change needs to be applied on top of your branch.

lxsmbadmin@netfsvm:~/latest_14mar/linux-fs$ git diff
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index af7483c246ac..447934ff80b8 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2969,6 +2969,12 @@ cifs_write_from_iter(loff_t offset, size_t len,
struct iov_iter *from,

                cur_len = min_t(const size_t, len, wsize);

+               if (!cur_len) {
+                       rc = -EAGAIN;
+                       add_credits_and_wake_if(server, credits, 0);
+                       break;
+               }
+
                wdata = cifs_writedata_alloc(cifs_uncached_writev_complete);
                if (!wdata) {
                        rc = -ENOMEM;

lxsmbadmin@netfsvm:~/xfstests-dev$ sudo ./check generic/013
SECTION       -- smb3
FSTYP         -- cifs
PLATFORM      -- Linux/x86_64 netfsvm 5.17.0-rc6+ #1 SMP PREEMPT Mon
Mar 14 09:05:47 UTC 2022
MKFS_OPTIONS  -- //127.0.0.1/sambashare_scratch
MOUNT_OPTIONS -- -ousername=,password=,noperm,vers=3.0,actimeo=0
//127.0.0.1/sambashare_scratch /mnt/xfstests_scratch

generic/013      149s
Ran: generic/013
Passed all 1 tests

SECTION       -- smb3
=========================
Ran: generic/013
Passed all 1 tests

Regards,
Rohith

On Fri, Mar 11, 2022 at 1:56 PM David Howells <dhowells@redhat.com> wrote:
>
>
> David Howells <dhowells@redhat.com> wrote:
>
> > The other issue is that if I run splice to an empty file, it works; running
> > another splice to the same file will result in the server giving
> > STATUS_ACCESS_DENIED when cifs_write_begin() tries to read from the file:
> >
> >     7 0.009485249  192.168.6.2 → 192.168.6.1  SMB2 183 Read Request Len:65536 Off:0 File: x
> >     8 0.009674245  192.168.6.1 → 192.168.6.2  SMB2 143 Read Response, Error: STATUS_ACCESS_DENIED
> >
> > Actually - that might be because the file is only 65536 bytes long because the
> > first splice finished short.
>
> Actually, it's because I opened the output file O_WRONLY.  If I open it
> O_RDWR, it works.  The test program is attached below.
>
> David
> ---
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <fcntl.h>
>
> int main(int argc, char *argv[])
> {
>         off64_t opos;
>         size_t len;
>         int in, out;
>
>         if (argc != 4) {
>                 printf("Format: %s size in out\n", argv[0]);
>                 exit(2);
>         }
>
>         len = atol(argv[1]);
>
>         if (strcmp(argv[2], "-") != 0) {
>                 in = open(argv[2], O_RDONLY);
>                 if (in < 0) {
>                         perror(argv[2]);
>                         return 1;
>                 }
>         } else {
>                 in = 0;
>         }
>
>         if (strcmp(argv[3], "-") != 0) {
>                 out = open(argv[3], O_WRONLY);  // Change to O_RDWR
>                 if (out < 0) {
>                         perror(argv[3]);
>                         return 1;
>                 }
>         } else {
>                 out = 1;
>         }
>
>         opos = 3;
>         if (splice(in, NULL, out, &opos, len, 0) < 0) {
>                 perror("splice");
>                 return 1;
>         }
>
>         if (close(in) < 0) {
>                 perror("close/in");
>                 return 1;
>         }
>
>         if (close(out) < 0) {
>                 perror("close/out");
>                 return 1;
>         }
>
>         return 0;
> }
>

Re: cifs conversion to netfslib

[Rohith Surabattula]

Hi David,

I noticed 2 other issues while running xfstests.

Noticed kernel OOPS during test generic/286:
folio_test_writeback returned false which means PG_writeback flag has
been cleared. I am not sure whether head page has PG_writeback flag
set initially? Can you please confirm.

[ 2275.941096] CIFS: bad 2000 @64f0000 page 64f0 64f1
[ 2275.945785] ------------[ cut here ]------------
[ 2275.945787] kernel BUG at
/home/lxsmbadmin/latest_14mar/linux-fs/fs/cifs/cifssmb.c:1954!
[ 2275.952198] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[ 2275.956500] CPU: 0 PID: 3369 Comm: kworker/0:7 Tainted: G
OE     5.17.0-rc6+ #1
[ 2275.962573] Hardware name: Microsoft Corporation Virtual
Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 10/27/2020
[ 2275.969812] Workqueue: cifsiod cifs_writev_complete [cifs]
[ 2275.974909] RIP: 0010:cifs_pages_written_back+0x1e1/0x1f0 [cifs]
[ 2275.975570] CIFS: bad 2000 @64f0000 page 64f0 64f1
[ 2275.980619] Code: 00 48 8b 07 f6 c4 04 0f 84 92 84 06 00 e8 77 a6
e6 db 48 89 c1 49 89 d8 4c 89 e2 44 89 ee 48 c7 c7 e0 f5 8e c0 e8 a4
9d 71 dc <0f> 0b e8 58 f5 76 dc 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00
55 48
[ 2275.980622] RSP: 0018:ffffac614177fdc8 EFLAGS: 00010246
[ 2275.980625] RAX: 0000000000000026 RBX: 00000000000064f1 RCX: 0000000000000000
[ 2275.980626] RDX: 0000000000000000 RSI: ffffffff9d767af1 RDI: 00000000ffffffff
[ 2275.980628] RBP: ffffac614177fe28 R08: 0000000000000000 R09: ffffac614177fbc0
[ 2275.980629] R10: ffffac614177fbb8 R11: ffffffff9df52b68 R12: 00000000064f0000
[ 2275.980631] R13: 0000000000002000 R14: 0000000000000000 R15: ffff9c8193bc7e40
[ 2275.980632] FS:  0000000000000000(0000) GS:ffff9c8337c00000(0000)
knlGS:0000000000000000
[ 2275.980636] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2275.980638] CR2: 00007fd099e56b90 CR3: 00000001028e4006 CR4: 00000000003706f0
[ 2275.980639] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2275.980640] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 2275.980641] Call Trace:
[ 2275.980643]  <TASK>
[ 2275.980648]  cifs_writev_complete+0x43d/0x500 [cifs]

Noticed that with netfs integration, file open with O_DIRECT flag is
not supported.

Regards,
Rohith

On Tue, Mar 15, 2022 at 9:20 AM Rohith Surabattula
<rohiths.msft@gmail.com> wrote:
>
> Hi David,
>
> Below change needs to be applied on top of your branch.
>
> lxsmbadmin@netfsvm:~/latest_14mar/linux-fs$ git diff
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index af7483c246ac..447934ff80b8 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2969,6 +2969,12 @@ cifs_write_from_iter(loff_t offset, size_t len,
> struct iov_iter *from,
>
>                 cur_len = min_t(const size_t, len, wsize);
>
> +               if (!cur_len) {
> +                       rc = -EAGAIN;
> +                       add_credits_and_wake_if(server, credits, 0);
> +                       break;
> +               }
> +
>                 wdata = cifs_writedata_alloc(cifs_uncached_writev_complete);
>                 if (!wdata) {
>                         rc = -ENOMEM;
>
> lxsmbadmin@netfsvm:~/xfstests-dev$ sudo ./check generic/013
> SECTION       -- smb3
> FSTYP         -- cifs
> PLATFORM      -- Linux/x86_64 netfsvm 5.17.0-rc6+ #1 SMP PREEMPT Mon
> Mar 14 09:05:47 UTC 2022
> MKFS_OPTIONS  -- //127.0.0.1/sambashare_scratch
> MOUNT_OPTIONS -- -ousername=,password=,noperm,vers=3.0,actimeo=0
> //127.0.0.1/sambashare_scratch /mnt/xfstests_scratch
>
> generic/013      149s
> Ran: generic/013
> Passed all 1 tests
>
> SECTION       -- smb3
> =========================
> Ran: generic/013
> Passed all 1 tests
>
> Regards,
> Rohith
>
> On Fri, Mar 11, 2022 at 1:56 PM David Howells <dhowells@redhat.com> wrote:
> >
> >
> > David Howells <dhowells@redhat.com> wrote:
> >
> > > The other issue is that if I run splice to an empty file, it works; running
> > > another splice to the same file will result in the server giving
> > > STATUS_ACCESS_DENIED when cifs_write_begin() tries to read from the file:
> > >
> > >     7 0.009485249  192.168.6.2 → 192.168.6.1  SMB2 183 Read Request Len:65536 Off:0 File: x
> > >     8 0.009674245  192.168.6.1 → 192.168.6.2  SMB2 143 Read Response, Error: STATUS_ACCESS_DENIED
> > >
> > > Actually - that might be because the file is only 65536 bytes long because the
> > > first splice finished short.
> >
> > Actually, it's because I opened the output file O_WRONLY.  If I open it
> > O_RDWR, it works.  The test program is attached below.
> >
> > David
> > ---
> > #define _GNU_SOURCE
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <unistd.h>
> > #include <fcntl.h>
> >
> > int main(int argc, char *argv[])
> > {
> >         off64_t opos;
> >         size_t len;
> >         int in, out;
> >
> >         if (argc != 4) {
> >                 printf("Format: %s size in out\n", argv[0]);
> >                 exit(2);
> >         }
> >
> >         len = atol(argv[1]);
> >
> >         if (strcmp(argv[2], "-") != 0) {
> >                 in = open(argv[2], O_RDONLY);
> >                 if (in < 0) {
> >                         perror(argv[2]);
> >                         return 1;
> >                 }
> >         } else {
> >                 in = 0;
> >         }
> >
> >         if (strcmp(argv[3], "-") != 0) {
> >                 out = open(argv[3], O_WRONLY);  // Change to O_RDWR
> >                 if (out < 0) {
> >                         perror(argv[3]);
> >                         return 1;
> >                 }
> >         } else {
> >                 out = 1;
> >         }
> >
> >         opos = 3;
> >         if (splice(in, NULL, out, &opos, len, 0) < 0) {
> >                 perror("splice");
> >                 return 1;
> >         }
> >
> >         if (close(in) < 0) {
> >                 perror("close/in");
> >                 return 1;
> >         }
> >
> >         if (close(out) < 0) {
> >                 perror("close/out");
> >                 return 1;
> >         }
> >
> >         return 0;
> > }
> >

Re: cifs conversion to netfslib

[David Howells]

Rohith Surabattula <rohiths.msft@gmail.com> wrote:

> I noticed 2 other issues while running xfstests.
> 
> Noticed kernel OOPS during test generic/286:
> folio_test_writeback returned false which means PG_writeback flag has
> been cleared. I am not sure whether head page has PG_writeback flag
> set initially? Can you please confirm.
> 
> [ 2275.941096] CIFS: bad 2000 @64f0000 page 64f0 64f1
> [ 2275.945785] ------------[ cut here ]------------
> [ 2275.945787] kernel BUG at /home/lxsmbadmin/latest_14mar/linux-fs/fs/cifs/cifssmb.c:1954!
> ...
> [ 2275.969812] Workqueue: cifsiod cifs_writev_complete [cifs]
> [ 2275.974909] RIP: 0010:cifs_pages_written_back+0x1e1/0x1f0 [cifs]
> [ 2275.975570] CIFS: bad 2000 @64f0000 page 64f0 64f1
> [ 2275.980641] Call Trace:
> [ 2275.980643]  <TASK>
> [ 2275.980648]  cifs_writev_complete+0x43d/0x500 [cifs]

I don't see that, but it fails in some other ways.  I think the bits should be
set.

I am seeing the occasional:

	CIFS: trying to dequeue a deleted mid

but I haven't managed to work out how I get to that yet.

I'm also occasionally seeing cifs_open() return a number >0, which causes all
sorts of fun. 

> Noticed that with netfs integration, file open with O_DIRECT flag is
> not supported.

It should be.  It jumps off to netfs_direct_read_iter() in various places.

openat(AT_FDCWD, "/xfstest.test/hello2", O_RDWR|O_DIRECT) = 3
fstatfs(3, {f_type=SMB2_MAGIC_NUMBER, f_bsize=1024, f_blocks=54961016, f_bfree=23691924, f_bavail=23691924, f_files=0, f_ffree=0, f_fsid={val=[1904023890, 0]}, f_namelen=255, f_frsize=1024, f_flags=ST_VALID|ST_RELATIME}) = 0
fstat(3, {st_mode=S_IFREG|0755, st_size=278528, ...}) = 0
pread64(3, "\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253\253"..., 4096, 0) = 4096

Btw, I've pushed an update to my cifs-experimental branch.

David

Re: cifs conversion to netfslib

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> I am seeing the occasional:
> 
> 	CIFS: trying to dequeue a deleted mid
> 
> but I haven't managed to work out how I get to that yet.

That turned out to be due to EFAULT occurring in sock_recvmsg() called from
cifs_readv_from_socket() because it was handed an iovec-class iov_iter.  It
went through:

		if (length <= 0) {
			cifs_dbg(FYI, "Received no data or error: %d\n", length);
			cifs_reconnect(server, false);
			return -ECONNABORTED;
		}

which called cifs_abort_connection() which forcibly marked the MIDs as
deleted.  However, they got dequeued later which triggered the message.

Telling netfs's DIO read to turn it into a bvec-class iov_iter instead stopped
that happening, but it's may be a latent problem.

I also found the causes of the occasional "server overflowed SMB3 credits"
messages that I've been seeing:

Firstly, the data read path was returning credits both when freeing the
subrequest and at the end of smb2_readv_callback().  I made the former
conditional on not doing the latter.

Secondly, cifs_write_back_from_locked_page() was returning credits, even if
->async_writev() was successful.  I made that only do it on error.

And now it gets through generic/013 for me.

David

Re: cifs conversion to netfslib

[David Howells]

Hi Rohith, Steve,

I've updated my cifs-experimental branch.  What I have there seems to work
much the same as without the patches.

I've managed to run some xfstests on it.  I note that various xfstests fail,
even without my patches, and some of them seem quite slow, again even without
my patches.

Note that I'm comparing the speed to afs which does a lot of directory
management locally compared to other network filesystems, so I might be
comparing apples and oranges.  For example, I can run generic/013 on afs in
4-7s, whereas it's 3m-7m on cifs.  However, since /013 does a bunch of
directory ops, afs probably has an advantage by caching the entire dir
contents locally, thereby satisfying lookup and readdir from local cache and
using a bulk status fetch to stat files from a dir in batches of 50 or so.
This is probably worth further investigation at some point.

David

Re: cifs conversion to netfslib

[Steve French]

On Thu, Mar 17, 2022 at 7:21 PM David Howells <dhowells@redhat.com> wrote:
>
> Hi Rohith, Steve,
>
> I've updated my cifs-experimental branch.  What I have there seems to work
> much the same as without the patches.
>
> I've managed to run some xfstests on it.  I note that various xfstests fail,
> even without my patches, and some of them seem quite slow, again even without
> my patches.
>
> Note that I'm comparing the speed to afs which does a lot of directory
> management locally compared to other network filesystems, so I might be
> comparing apples and oranges.  For example, I can run generic/013 on afs in
> 4-7s, whereas it's 3m-7m on cifs.  However, since /013 does a bunch of
> directory ops, afs probably has an advantage by caching the entire dir
> contents locally, thereby satisfying lookup and readdir from local cache and
> using a bulk status fetch to stat files from a dir in batches of 50 or so.
> This is probably worth further investigation at some point.

Yes - this is a point that Nagendra also made recently that we could
benefit from
some of the tricks that NFS uses for caching directory contents not just the
stat and revalidate_dentry info (e.g. affected by actimeo).

Since the protocol supports directory leases it would be relatively
safe compared
to some other protocols to cache directory contents much more aggressively,
and even without directory leases, other tricks (like directory change
notification
or even simply the mtime on the directory) can be used to cache directory
contents reasonably safely.


-- 
Thanks,

Steve

Re: cifs conversion to netfslib

[Rohith Surabattula]

Hi Dave,

As I mentioned earlier some tests are skipped with reason(O_DIRECT is
not supported) with netfs changes. This is because "ctx->len" is
getting updated to 0 in the code below for the DIRECT IO case.

Snippet from cifs_writev:
        rc = extract_iter_to_iter(from, ctx->len, &ctx->iter, &ctx->bv);
        if (rc < 0) {
                kref_put(&ctx->refcount, cifs_aio_ctx_release);
                return rc;
        }
        ctx->npages = rc;
        ctx->len = iov_iter_count(&ctx->iter);


lxsmbadmin@netfsvm:~/xfstests-dev$ sudo ./check generic/091
SECTION       -- smb3
FSTYP         -- cifs
PLATFORM      -- Linux/x86_64 netfsvm 5.17.0-rc6+ #1 SMP PREEMPT Fri
Mar 18 05:55:29 UTC 2022
MKFS_OPTIONS  -- //127.0.0.1/sambashare_scratch
MOUNT_OPTIONS --
-ousername=lxsmbadmin,password=RedBagBesideALake!,noperm,vers=3.0,actimeo=0
//127.0.0.1/sambashare_scratch /mnt/xfstests_scratch

generic/091     [not run] O_DIRECT is not supported
Ran: generic/091
Not run: generic/091
Passed all 1 tests

SECTION       -- smb3
=========================
Ran: generic/091
Not run: generic/091
Passed all 1 tests

Regards,
Rohith

On Fri, Mar 18, 2022 at 8:22 AM Steve French <smfrench@gmail.com> wrote:
>
> On Thu, Mar 17, 2022 at 7:21 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Hi Rohith, Steve,
> >
> > I've updated my cifs-experimental branch.  What I have there seems to work
> > much the same as without the patches.
> >
> > I've managed to run some xfstests on it.  I note that various xfstests fail,
> > even without my patches, and some of them seem quite slow, again even without
> > my patches.
> >
> > Note that I'm comparing the speed to afs which does a lot of directory
> > management locally compared to other network filesystems, so I might be
> > comparing apples and oranges.  For example, I can run generic/013 on afs in
> > 4-7s, whereas it's 3m-7m on cifs.  However, since /013 does a bunch of
> > directory ops, afs probably has an advantage by caching the entire dir
> > contents locally, thereby satisfying lookup and readdir from local cache and
> > using a bulk status fetch to stat files from a dir in batches of 50 or so.
> > This is probably worth further investigation at some point.
>
> Yes - this is a point that Nagendra also made recently that we could
> benefit from
> some of the tricks that NFS uses for caching directory contents not just the
> stat and revalidate_dentry info (e.g. affected by actimeo).
>
> Since the protocol supports directory leases it would be relatively
> safe compared
> to some other protocols to cache directory contents much more aggressively,
> and even without directory leases, other tricks (like directory change
> notification
> or even simply the mtime on the directory) can be used to cache directory
> contents reasonably safely.
>
>
> --
> Thanks,
>
> Steve

Re: cifs conversion to netfslib

[David Howells]

Rohith Surabattula <rohiths.msft@gmail.com> wrote:

> Snippet from cifs_writev:
>         rc = extract_iter_to_iter(from, ctx->len, &ctx->iter, &ctx->bv);
>         if (rc < 0) {
>                 kref_put(&ctx->refcount, cifs_aio_ctx_release);
>                 return rc;
>         }
>         ctx->npages = rc;
>         ctx->len = iov_iter_count(&ctx->iter);

Bah.  ctx->len is 0 at the point of the extraction.  That should be:

	rc = extract_iter_to_iter(from, iov_iter_count(from), &ctx->iter, &ctx->bv);

David