We have a simple repro we are looking at now with 4 channels
We start with reasonable number of credits but end up with channels 3
and 4 reconnecting and only having one credit on reconnect
On Wed, Apr 28, 2021 at 10:23 AM Aurélien Aptel <aaptel@xxxxxxxx> wrote:
>
> Shyam Prasad N <nspmangalore@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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)
>
--
Thanks,
Steve
