Re: [PATCH] cifs: when out of credits on one channel, retry on another.

["Aurélien Aptel" <aaptel@suse.com>]

Shyam Prasad N <nspmangalore@gmail.com> writes:
> The idea is to retry on -EDEADLK (which we return when we run out of
> credits and we have no requests in flight), so that another channel is
> chosen for the next request.

I agree with the idea.

Have you been able to test those DEADLK codepaths? If so how, because we
should add buildbot tests I think.

For the function renaming, there is a precedent in the kernel to use a _
prefix for "sub-functions" instead of the _final suffix.

> This fix mostly introduces a wrapper for all functions which call
> cifs_pick_channel. In the wrapper function, the function is retried
> when the error is -EDEADLK, and uses multichannel.

I think we should put a limit on the number of retries. If it goes above
some reasonable number print a warning and return EDEADLK.

We could also hide the retry logic loop in a macro to avoid code
duplication when possible. This could get rid of multiple simpler funcs
I think if we use the macro in the caller.

Something like (feel free to improve/modify):

#define MULTICHAN_RETRY(chan_count, call) \
({ \
	int __rc; \
	int __tries = 0;
	do { \
		__rc = call; \
	        __tries++; \
	} while (__tries < MULTICHAN_MAX_RETRY && __rc == -EDEADLK && chan_count > 1) \
	WARN_ON(__tries == MULTICHAN_MAX_RETRY); \
	__rc; \
})        


> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/cifs/file.c      |  96 +++++++++++-
>  fs/cifs/smb2inode.c |  93 ++++++++----
>  fs/cifs/smb2ops.c   | 113 +++++++++++++-
>  fs/cifs/smb2pdu.c   | 359 ++++++++++++++++++++++++++++++++++++++++----
>  4 files changed, 593 insertions(+), 68 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 7e97aeabd616..7609b9739ce4 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2378,7 +2378,7 @@ wdata_send_pages(struct cifs_writedata *wdata, unsigned int nr_pages,
>  	return rc;
>  }
>  
> -static int cifs_writepages(struct address_space *mapping,
> +static int cifs_writepages_final(struct address_space *mapping,
>  			   struct writeback_control *wbc)
>  {
>  	struct inode *inode = mapping->host;
> @@ -2543,6 +2543,21 @@ static int cifs_writepages(struct address_space *mapping,
>  	return rc;
>  }
>  
> +static int cifs_writepages(struct address_space *mapping,
> +			   struct writeback_control *wbc)
> +{
> +	int rc;
> +	struct inode *inode = mapping->host;
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> +	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> +
> +	do {
> +		rc = cifs_writepages_final(mapping, wbc); 
> +	} while (tcon->ses->chan_count > 1 && rc == -EDEADLK);
> +
> +	return rc;
> +}

cifs_writepages can issue multiple writes. I suspect it can do some
successful writes before it runs out of credit, and I'm not sure if
individual pages are marked as flushed or not. Basically this func might
not be idempotent so that's one codepath that we should definitely try I
think (unless someone knows more and can explain me).

Same goes for /some/ of the the other funcs... I think this should be
documented/tried.
>  
> +static int
> +smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> +		 struct cifs_sb_info *cifs_sb, const char *full_path,
> +		 __u32 desired_access, __u32 create_disposition,
> +		 __u32 create_options, umode_t mode, void *ptr, int command,
> +		 bool check_open, bool writable_path)
> +{

I would use an int for flags intead of passing those 2 booleans. This
way adding more flags in the future will be easier, and you can use
named enum or defines in the code instead of true/false which will be
more understandable.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)