| Message ID | 20201006022623.21026-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: handle -EINTR in cifs_setattr | expand |
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 >
Question: What causes cifs_setattr_* to return -EINTR? Does an infinite reattempt make sense here? Or should we give up after N attempts? 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
Ignore my second question. I got the answer in your description. On Tue, Oct 6, 2020 at 10:36 AM 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? > 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
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
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
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(-)