On Tue, Oct 6, 2020 at 3:06 PM Shyam Prasad N <nspmangalore@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> >
> > tentatively merged into cifs-2.6.git for-next
> >
> > On Mon, Oct 5, 2020 at 9:26 PM Ronnie Sahlberg <lsahlber@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
