Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[ronnie sahlberg <ronniesahlberg@gmail.com>]

On Tue, Oct 6, 2020 at 3:06 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Question: What causes cifs_setattr_* to return -EINTR?
> Does an infinite reattempt make sense here? Or should we give up after
> N attempts?

Good question. Probably.
I think you are right we might need to limit the retries.

One  case could be where we never receive a response from the server
and userspace
wants to CTRL-C the thread.  It might mean we have to do N CTRL-C to
end the process but N is better than infinite CTRL-C :-)

I will resend with a limit on retries.

Thanks!

> Also, should we cifsFYI log before we re-attempt?
>
> Regards,
> Shyam
>
> On Tue, Oct 6, 2020 at 10:03 AM Steve French <smfrench@gmail.com> wrote:
> >
> > tentatively merged into cifs-2.6.git for-next
> >
> > On Mon, Oct 5, 2020 at 9:26 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > RHBZ: 1848178
> > >
> > > Some calls that set attributes, like utimensat(), are not supposed to return
> > > -EINTR and thus do not have handlers for this in glibc which causes us
> > > to leak -EINTR to the applications which are also unprepared to handle it.
> > >
> > > For example tar will break if utimensat() return -EINTR and abort unpacking
> > > the archive. Other applications may break too.
> > >
> > > To handle this we add checks, and retry, for -EINTR in cifs_setattr()
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/inode.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > > index 3989d08396ac..74ed12f2c8aa 100644
> > > --- a/fs/cifs/inode.c
> > > +++ b/fs/cifs/inode.c
> > > @@ -2879,13 +2879,17 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
> > >  {
> > >         struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
> > >         struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
> > > +       int rc;
> > >
> > > -       if (pTcon->unix_ext)
> > > -               return cifs_setattr_unix(direntry, attrs);
> > > -
> > > -       return cifs_setattr_nounix(direntry, attrs);
> > > +       do {
> > > +               if (pTcon->unix_ext)
> > > +                       rc = cifs_setattr_unix(direntry, attrs);
> > > +               else
> > > +                       rc = cifs_setattr_nounix(direntry, attrs);
> > > +       } while (rc == -EINTR);
> > >
> > >         /* BB: add cifs_setattr_legacy for really old servers */
> > > +       return rc;
> > >  }
> > >
> > >  #if 0
> > > --
> > > 2.13.6
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> -Shyam