[PATCH] [CIFS]: Add clamp_length support

[PATCH] [CIFS]: Add clamp_length support

[Rohith Surabattula]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Hi All,

Added clamp length handler to further slice the read requests based on
available credits.

This patch needs to be applied on top of Dave howells 7-patch
series(netfs integration for cifs).

@David Howells As the above 7-patch series is dependent on netfs lib
changes which are not merged yet. Please let us know the timelines for
mege process.

Regards,
Rohith

[-- Attachment #2: CIFS-Add-clamp_length-support.patch --]
[-- Type: application/octet-stream, Size: 4579 bytes --]

From e0eee5ff329273985e41e3ae9feb6c27dcfdb419 Mon Sep 17 00:00:00 2001
From: Rohith Surabattula <rohiths@microsoft.com>
Date: Tue, 8 Feb 2022 05:38:29 +0000
Subject: [PATCH] [CIFS]: Add clamp_length support

Added clamp length handler to slice the read requests further based on available credits.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
---
 fs/cifs/file.c        | 40 ++++++++++++++++++++++++++++++++--------
 fs/cifs/smb2ops.c     |  1 +
 include/linux/netfs.h |  1 +
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 36559de02e37..8f4961960beb 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3459,11 +3459,10 @@ static void cifs_req_issue_op(struct netfs_read_subrequest *subreq)
 	struct cifs_readdata *rdata;
 	struct cifsFileInfo *open_file = rreq->netfs_priv;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(rreq->inode->i_sb);
-	struct cifs_credits credits_on_stack, *credits = &credits_on_stack;
+	struct cifs_credits *credits;
 	unsigned int xid;
 	pid_t pid;
 	int rc = 0;
-	unsigned int rsize;
 
 	xid = get_xid();
 
@@ -3478,18 +3477,18 @@ static void cifs_req_issue_op(struct netfs_read_subrequest *subreq)
 		 __func__, rreq->debug_id, subreq->debug_index, rreq->mapping,
 		 subreq->transferred, subreq->len);
 
+	credits = subreq->subreq_priv;
+
 	if (open_file->invalidHandle) {
 		do {
 			rc = cifs_reopen_file(open_file, true);
 		} while (rc == -EAGAIN);
-		if (rc)
+		if (rc) {
+			add_credits_and_wake_if(server, credits, 0);
 			goto out;
+		}
 	}
 
-	rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize, &rsize, credits);
-	if (rc)
-		goto out;
-
 	rdata = cifs_readdata_alloc(NULL);
 	if (!rdata) {
 		add_credits_and_wake_if(server, credits, 0);
@@ -3504,7 +3503,7 @@ static void cifs_req_issue_op(struct netfs_read_subrequest *subreq)
 	rdata->offset	= subreq->start + subreq->transferred;
 	rdata->bytes	= subreq->len   - subreq->transferred;
 	rdata->pid	= pid;
-	rdata->credits	= credits_on_stack;
+	rdata->credits	= *credits;
 	rdata->iter	= subreq->iter;
 
 	rc = adjust_credits(server, &rdata->credits, rdata->bytes);
@@ -3526,6 +3525,7 @@ static void cifs_req_issue_op(struct netfs_read_subrequest *subreq)
 
 out:
 	free_xid(xid);
+	kfree(credits);
 	if (rc)
 		netfs_subreq_terminated(subreq, rc, false);
 }
@@ -3566,6 +3566,29 @@ static void cifs_expand_readahead(struct netfs_read_request *rreq)
 		rreq->len = i_size - rreq->start;
 }
 
+static bool cifs_clamp_length(struct netfs_read_subrequest *subreq)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(subreq->rreq->inode->i_sb);
+	struct TCP_Server_Info *server;
+	struct cifsFileInfo *open_file = subreq->rreq->netfs_priv;
+	struct cifs_credits *credits;
+	unsigned int rsize;
+	int rc;
+
+	credits = kmalloc(sizeof(struct cifs_credits), GFP_KERNEL);
+	server = cifs_pick_channel(tlink_tcon(open_file->tlink)->ses);
+
+	rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize,
+                                                   &rsize, credits);
+	if (rc)
+		return false;
+
+	subreq->len = rsize;
+	subreq->subreq_priv = credits;
+	return true;
+}
+
+
 static void cifs_rreq_done(struct netfs_read_request *rreq)
 {
 	struct inode *inode = rreq->inode;
@@ -3586,6 +3609,7 @@ const struct netfs_request_ops cifs_req_ops = {
 	.init_rreq		= cifs_init_rreq,
 	.expand_readahead	= cifs_expand_readahead,
 	.issue_op		= cifs_req_issue_op,
+	.clamp_length           = cifs_clamp_length,
 	.done			= cifs_rreq_done,
 	.cleanup		= cifs_req_cleanup,
 };
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index e1649ac194db..5faf45672891 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -4917,6 +4917,7 @@ handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
 		length = copy_to_iter(buf + data_offset, data_len, &rdata->iter);
 		if (length < 0)
 			return length;
+		rdata->got_bytes = data_len;
 	} else {
 		/* read response payload cannot be in both buf and pages */
 		WARN_ONCE(1, "buf can not contain only a part of read data");
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index b05b7a7d90e6..e8fb1b3694c6 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -170,6 +170,7 @@ struct netfs_read_subrequest {
 #define NETFS_SREQ_SHORT_READ		2	/* Set if there was a short read from the cache */
 #define NETFS_SREQ_SEEK_DATA_READ	3	/* Set if ->read() should SEEK_DATA first */
 #define NETFS_SREQ_NO_PROGRESS		4	/* Set if we didn't manage to read any data */
+	void                    *subreq_priv;
 };
 
 enum netfs_read_origin {
-- 
2.32.0

Re: [PATCH] [CIFS]: Add clamp_length support

[David Howells]

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

> +	credits = kmalloc(sizeof(struct cifs_credits), GFP_KERNEL);
> ...
> +	subreq->subreq_priv = credits;

Would it be better if I made it so that the netfs could specify the size of
the netfs_read_subrequest struct to be allocated, thereby allowing it to tag
extra data on the end?

David

Re: [PATCH] [CIFS]: Add clamp_length support

[Rohith Surabattula]

Hi David,

Do you mean the clamp handler in netfs should return the size of data
to be allocated instead of allocating itself ?

Regards,
Rohith

On Thu, Feb 17, 2022 at 2:56 AM David Howells <dhowells@redhat.com> wrote:
>
> Rohith Surabattula <rohiths.msft@gmail.com> wrote:
>
> > +     credits = kmalloc(sizeof(struct cifs_credits), GFP_KERNEL);
> > ...
> > +     subreq->subreq_priv = credits;
>
> Would it be better if I made it so that the netfs could specify the size of
> the netfs_read_subrequest struct to be allocated, thereby allowing it to tag
> extra data on the end?
>
> David
>

Re: [PATCH] [CIFS]: Add clamp_length support

[David Howells]

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

> > Rohith Surabattula <rohiths.msft@gmail.com> wrote:
> >
> > > +     credits = kmalloc(sizeof(struct cifs_credits), GFP_KERNEL);
> > > ...
> > > +     subreq->subreq_priv = credits;
> >
> > Would it be better if I made it so that the netfs could specify the size of
> > the netfs_read_subrequest struct to be allocated, thereby allowing it to tag
> > extra data on the end?
>
> Do you mean the clamp handler in netfs should return the size of data
> to be allocated instead of allocating itself ?

No, I was thinking of putting a size_t in struct netfs_request_ops that
indicates how big the subrequest struct should be:

	struct netfs_request_ops {
		...
		size_t subrequest_size;
	};

and then:

	struct netfs_read_subrequest *netfs_alloc_subrequest(
		struct netfs_read_request *rreq)
	{
		struct netfs_read_subrequest *subreq;

		subreq = kzalloc(rreq->ops->subrequest_size, GFP_KERNEL);
		if (subreq) {
			INIT_LIST_HEAD(&subreq->rreq_link);
			refcount_set(&subreq->usage, 2);
			subreq->rreq = rreq;
			netfs_get_read_request(rreq);
			netfs_stat(&netfs_n_rh_sreq);
		}

		return subreq;
	}

This would allow you to do, for instance:

	struct cifs_subrequest {
		struct netfs_read_subrequest subreq;
		struct cifs_credits credits;
	};

then:

	const struct netfs_request_ops cifs_req_ops = {
		.subrequest_size	= sizeof(struct cifs_subrequest),
		.init_rreq		= cifs_init_rreq,
		.expand_readahead	= cifs_expand_readahead,
		.clamp_length		= cifs_clamp_length,
		.issue_op		= cifs_req_issue_op,
		.done			= cifs_rreq_done,
		.cleanup		= cifs_req_cleanup,
	};

and then:

	static bool cifs_clamp_length(struct netfs_read_subrequest *subreq)
	{
		struct cifs_subrequest *cifs_subreq =
			container_of(subreq, struct cifs_subrequest, subreq);
		struct cifs_sb_info *cifs_sb = CIFS_SB(subreq->rreq->inode->i_sb);
		struct TCP_Server_Info *server;
		struct cifsFileInfo *open_file = subreq->rreq->netfs_priv;
		struct cifs_credits *credits = &cifs_subreq->credits;
		unsigned int rsize;
		int rc;

		server = cifs_pick_channel(tlink_tcon(open_file->tlink)->ses);

		rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize,
	                                                   &rsize, credits);
		if (rc)
			return false;

		subreq->len = rsize;
		return true;
	}

David

Re: [PATCH] [CIFS]: Add clamp_length support

[Rohith Surabattula]

Yes, It will look better. Agree with your idea. I will work on it.

Regards,
Rohith

On Mon, Feb 28, 2022 at 8:09 PM David Howells <dhowells@redhat.com> wrote:
>
> Rohith Surabattula <rohiths.msft@gmail.com> wrote:
>
> > > Rohith Surabattula <rohiths.msft@gmail.com> wrote:
> > >
> > > > +     credits = kmalloc(sizeof(struct cifs_credits), GFP_KERNEL);
> > > > ...
> > > > +     subreq->subreq_priv = credits;
> > >
> > > Would it be better if I made it so that the netfs could specify the size of
> > > the netfs_read_subrequest struct to be allocated, thereby allowing it to tag
> > > extra data on the end?
> >
> > Do you mean the clamp handler in netfs should return the size of data
> > to be allocated instead of allocating itself ?
>
> No, I was thinking of putting a size_t in struct netfs_request_ops that
> indicates how big the subrequest struct should be:
>
>         struct netfs_request_ops {
>                 ...
>                 size_t subrequest_size;
>         };
>
> and then:
>
>         struct netfs_read_subrequest *netfs_alloc_subrequest(
>                 struct netfs_read_request *rreq)
>         {
>                 struct netfs_read_subrequest *subreq;
>
>                 subreq = kzalloc(rreq->ops->subrequest_size, GFP_KERNEL);
>                 if (subreq) {
>                         INIT_LIST_HEAD(&subreq->rreq_link);
>                         refcount_set(&subreq->usage, 2);
>                         subreq->rreq = rreq;
>                         netfs_get_read_request(rreq);
>                         netfs_stat(&netfs_n_rh_sreq);
>                 }
>
>                 return subreq;
>         }
>
> This would allow you to do, for instance:
>
>         struct cifs_subrequest {
>                 struct netfs_read_subrequest subreq;
>                 struct cifs_credits credits;
>         };
>
> then:
>
>         const struct netfs_request_ops cifs_req_ops = {
>                 .subrequest_size        = sizeof(struct cifs_subrequest),
>                 .init_rreq              = cifs_init_rreq,
>                 .expand_readahead       = cifs_expand_readahead,
>                 .clamp_length           = cifs_clamp_length,
>                 .issue_op               = cifs_req_issue_op,
>                 .done                   = cifs_rreq_done,
>                 .cleanup                = cifs_req_cleanup,
>         };
>
> and then:
>
>         static bool cifs_clamp_length(struct netfs_read_subrequest *subreq)
>         {
>                 struct cifs_subrequest *cifs_subreq =
>                         container_of(subreq, struct cifs_subrequest, subreq);
>                 struct cifs_sb_info *cifs_sb = CIFS_SB(subreq->rreq->inode->i_sb);
>                 struct TCP_Server_Info *server;
>                 struct cifsFileInfo *open_file = subreq->rreq->netfs_priv;
>                 struct cifs_credits *credits = &cifs_subreq->credits;
>                 unsigned int rsize;
>                 int rc;
>
>                 server = cifs_pick_channel(tlink_tcon(open_file->tlink)->ses);
>
>                 rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize,
>                                                            &rsize, credits);
>                 if (rc)
>                         return false;
>
>                 subreq->len = rsize;
>                 return true;
>         }
>
> David
>

Re: [PATCH] [CIFS]: Add clamp_length support

[David Howells]

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

> >         struct netfs_read_subrequest *netfs_alloc_subrequest(

Btw, note the patches I just posted that rename these structs and some of the
functions.

David

Re: [PATCH] [CIFS]: Add clamp_length support

[Rohith Surabattula]

Hi David,

Patches which you posted today are on top of 7-patch series "[RFC][RFC
PATCH 0/7] cifs: In-progress conversion to use iov_iters and netfslib"
which you published earlier?

or this 7-patch series needs to be reworked ?

Regards,
Rohith

On Wed, Mar 2, 2022 at 8:46 PM David Howells <dhowells@redhat.com> wrote:
>
> Rohith Surabattula <rohiths.msft@gmail.com> wrote:
>
> > >         struct netfs_read_subrequest *netfs_alloc_subrequest(
>
> Btw, note the patches I just posted that rename these structs and some of the
> functions.
>
> David
>

Re: [PATCH] [CIFS]: Add clamp_length support

[David Howells]

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

> Patches which you posted today are on top of 7-patch series "[RFC][RFC
> PATCH 0/7] cifs: In-progress conversion to use iov_iters and netfslib"
> which you published earlier?

They're a subset of the patches on my netfs-lib branch on top of which the
7-patch series was based.

> or this 7-patch series needs to be reworked ?

Yeah.  I'm going to have a look at that rebasing my netfs-lib and
cifs-experimental branches tomorrow.

David

Re: [PATCH] [CIFS]: Add clamp_length support

[David Howells]

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

> > No, I was thinking of putting a size_t in struct netfs_request_ops that
> > indicates how big the subrequest struct should be:
> ...
> Yes, It will look better. Agree with your idea. I will work on it.

See the patches I've just added to my cifs-experimental branch.

David