* [PATCH] cifs: handle -EINTR in cifs_setattr
@ 2020-10-06 5:26 Ronnie Sahlberg
2020-10-06 10:56 ` Aurélien Aptel
0 siblings, 1 reply; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-06 5:26 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
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 | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3989d08396ac..2dd6e7902ff4 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2879,13 +2879,18 @@ 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, retries = 0;
- 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);
+ retries++;
+ } while (rc == -EINTR && retries < 4);
/* BB: add cifs_setattr_legacy for really old servers */
+ return rc;
}
#if 0
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 5:26 [PATCH] cifs: handle -EINTR in cifs_setattr Ronnie Sahlberg
@ 2020-10-06 10:56 ` Aurélien Aptel
2020-10-06 22:22 ` ronnie sahlberg
0 siblings, 1 reply; 14+ messages in thread
From: Aurélien Aptel @ 2020-10-06 10:56 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French
Hi Ronnie,
Ronnie Sahlberg <lsahlber@redhat.com> writes:
> 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.
EINTR happens when the task receives a signal right?
https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
Given what you said and what the glibc doc reads it seems like the fix
should go in glibc. Otherwise we need to care about every single syscall.
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 10:56 ` Aurélien Aptel
@ 2020-10-06 22:22 ` ronnie sahlberg
2020-10-07 1:44 ` Steve French
2020-10-07 10:45 ` Aurélien Aptel
0 siblings, 2 replies; 14+ messages in thread
From: ronnie sahlberg @ 2020-10-06 22:22 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
On Tue, Oct 6, 2020 at 8:57 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Hi Ronnie,
>
> Ronnie Sahlberg <lsahlber@redhat.com> writes:
> > 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.
>
> EINTR happens when the task receives a signal right?
>
> https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
>
> Given what you said and what the glibc doc reads it seems like the fix
> should go in glibc. Otherwise we need to care about every single syscall.
glibc have handling for EINTR in most places, but not in for example utimensat()
because this function is not supposed to be able to return this error.
Similarly we have functions like chmod and chown that also come into cifs.ko
via the same entrypoint: cifs_setattr()
I think all of these "update inode data" are never supposed to be
interruptible since
they were classically just updating the in-memory inode and the thread
would never hit d-state.
Anyway, for these functions EINTR is not a valid return code so I
think we should take care to not
return it. Even if we change glibc adn the very very thin wrapper for
this functions there are applications
that might call the systemcall directly or via a different c-library.
>
> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 22:22 ` ronnie sahlberg
@ 2020-10-07 1:44 ` Steve French
2020-10-07 2:48 ` Steve French
2020-10-07 10:45 ` Aurélien Aptel
1 sibling, 1 reply; 14+ messages in thread
From: Steve French @ 2020-10-07 1:44 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Aurélien Aptel, Ronnie Sahlberg, linux-cifs
Tentatively merged updated version of this patch into cifs-2.6.git
for-next pending more testing and reviews
On Tue, Oct 6, 2020 at 5:22 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
>
> On Tue, Oct 6, 2020 at 8:57 PM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > Hi Ronnie,
> >
> > Ronnie Sahlberg <lsahlber@redhat.com> writes:
> > > 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.
> >
> > EINTR happens when the task receives a signal right?
> >
> > https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
> >
> > Given what you said and what the glibc doc reads it seems like the fix
> > should go in glibc. Otherwise we need to care about every single syscall.
>
> glibc have handling for EINTR in most places, but not in for example utimensat()
> because this function is not supposed to be able to return this error.
> Similarly we have functions like chmod and chown that also come into cifs.ko
> via the same entrypoint: cifs_setattr()
> I think all of these "update inode data" are never supposed to be
> interruptible since
> they were classically just updating the in-memory inode and the thread
> would never hit d-state.
>
> Anyway, for these functions EINTR is not a valid return code so I
> think we should take care to not
> return it. Even if we change glibc adn the very very thin wrapper for
> this functions there are applications
> that might call the systemcall directly or via a different c-library.
>
>
> >
> > 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-07 1:44 ` Steve French
@ 2020-10-07 2:48 ` Steve French
0 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2020-10-07 2:48 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Aurélien Aptel, Ronnie Sahlberg, linux-cifs
Also FYI- I made a minor modification after discussing with Ronnie,
reducing the retry count
On Tue, Oct 6, 2020 at 8:44 PM Steve French <smfrench@gmail.com> wrote:
>
> Tentatively merged updated version of this patch into cifs-2.6.git
> for-next pending more testing and reviews
>
> On Tue, Oct 6, 2020 at 5:22 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
> >
> > On Tue, Oct 6, 2020 at 8:57 PM Aurélien Aptel <aaptel@suse.com> wrote:
> > >
> > > Hi Ronnie,
> > >
> > > Ronnie Sahlberg <lsahlber@redhat.com> writes:
> > > > 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.
> > >
> > > EINTR happens when the task receives a signal right?
> > >
> > > https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
> > >
> > > Given what you said and what the glibc doc reads it seems like the fix
> > > should go in glibc. Otherwise we need to care about every single syscall.
> >
> > glibc have handling for EINTR in most places, but not in for example utimensat()
> > because this function is not supposed to be able to return this error.
> > Similarly we have functions like chmod and chown that also come into cifs.ko
> > via the same entrypoint: cifs_setattr()
> > I think all of these "update inode data" are never supposed to be
> > interruptible since
> > they were classically just updating the in-memory inode and the thread
> > would never hit d-state.
> >
> > Anyway, for these functions EINTR is not a valid return code so I
> > think we should take care to not
> > return it. Even if we change glibc adn the very very thin wrapper for
> > this functions there are applications
> > that might call the systemcall directly or via a different c-library.
> >
> >
> > >
> > > 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
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 22:22 ` ronnie sahlberg
2020-10-07 1:44 ` Steve French
@ 2020-10-07 10:45 ` Aurélien Aptel
2020-10-08 23:31 ` ronnie sahlberg
1 sibling, 1 reply; 14+ messages in thread
From: Aurélien Aptel @ 2020-10-07 10:45 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
ronnie sahlberg <ronniesahlberg@gmail.com> writes:
> glibc have handling for EINTR in most places, but not in for example utimensat()
> because this function is not supposed to be able to return this error.
> Similarly we have functions like chmod and chown that also come into cifs.ko
> via the same entrypoint: cifs_setattr()
> I think all of these "update inode data" are never supposed to be
> interruptible since
> they were classically just updating the in-memory inode and the thread
> would never hit d-state.
I see, thanks for the explanation.
> Anyway, for these functions EINTR is not a valid return code so I
> think we should take care to not
> return it. Even if we change glibc adn the very very thin wrapper for
> this functions there are applications
> that might call the systemcall directly or via a different c-library.
Should we use is_retryable_error() to check for the errcode?
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-07 10:45 ` Aurélien Aptel
@ 2020-10-08 23:31 ` ronnie sahlberg
0 siblings, 0 replies; 14+ messages in thread
From: ronnie sahlberg @ 2020-10-08 23:31 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
On Wed, Oct 7, 2020 at 8:45 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> ronnie sahlberg <ronniesahlberg@gmail.com> writes:
> > glibc have handling for EINTR in most places, but not in for example utimensat()
> > because this function is not supposed to be able to return this error.
> > Similarly we have functions like chmod and chown that also come into cifs.ko
> > via the same entrypoint: cifs_setattr()
> > I think all of these "update inode data" are never supposed to be
> > interruptible since
> > they were classically just updating the in-memory inode and the thread
> > would never hit d-state.
>
> I see, thanks for the explanation.
>
> > Anyway, for these functions EINTR is not a valid return code so I
> > think we should take care to not
> > return it. Even if we change glibc adn the very very thin wrapper for
> > this functions there are applications
> > that might call the systemcall directly or via a different c-library.
>
> Should we use is_retryable_error() to check for the errcode?
That is probably a good idea.
I will resend the patch with that suggestion.
Thanks!
>
> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] cifs: handle -EINTR in cifs_setattr
@ 2020-10-08 23:32 Ronnie Sahlberg
2020-10-09 5:43 ` Steve French
0 siblings, 1 reply; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-08 23:32 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
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 | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3989d08396ac..0bd22c41a623 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2879,13 +2879,18 @@ 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, retries = 0;
- 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);
+ retries++;
+ } while (is_retryable_error(rc) && retries < 2);
/* BB: add cifs_setattr_legacy for really old servers */
+ return rc;
}
#if 0
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-08 23:32 Ronnie Sahlberg
@ 2020-10-09 5:43 ` Steve French
0 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2020-10-09 5:43 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs
merged updated patch into cifs-2.6.git for-next
On Thu, Oct 8, 2020 at 6:33 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 | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 3989d08396ac..0bd22c41a623 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2879,13 +2879,18 @@ 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, retries = 0;
>
> - 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);
> + retries++;
> + } while (is_retryable_error(rc) && retries < 2);
>
> /* BB: add cifs_setattr_legacy for really old servers */
> + return rc;
> }
>
> #if 0
> --
> 2.13.6
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] cifs: handle -EINTR in cifs_setattr
@ 2020-10-06 2:26 Ronnie Sahlberg
2020-10-06 4:32 ` Steve French
0 siblings, 1 reply; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-06 2:26 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 2:26 Ronnie Sahlberg
@ 2020-10-06 4:32 ` Steve French
2020-10-06 5:06 ` Shyam Prasad N
0 siblings, 1 reply; 14+ messages in thread
From: Steve French @ 2020-10-06 4:32 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 4:32 ` Steve French
@ 2020-10-06 5:06 ` Shyam Prasad N
2020-10-06 5:09 ` Shyam Prasad N
2020-10-06 5:23 ` ronnie sahlberg
0 siblings, 2 replies; 14+ messages in thread
From: Shyam Prasad N @ 2020-10-06 5:06 UTC (permalink / raw)
To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 5:06 ` Shyam Prasad N
@ 2020-10-06 5:09 ` Shyam Prasad N
2020-10-06 5:23 ` ronnie sahlberg
1 sibling, 0 replies; 14+ messages in thread
From: Shyam Prasad N @ 2020-10-06 5:09 UTC (permalink / raw)
To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs
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
--
-Shyam
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] cifs: handle -EINTR in cifs_setattr
2020-10-06 5:06 ` Shyam Prasad N
2020-10-06 5:09 ` Shyam Prasad N
@ 2020-10-06 5:23 ` ronnie sahlberg
1 sibling, 0 replies; 14+ messages in thread
From: ronnie sahlberg @ 2020-10-06 5:23 UTC (permalink / raw)
To: Shyam Prasad N; +Cc: Steve French, Ronnie Sahlberg, linux-cifs
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-09 5:44 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 5:26 [PATCH] cifs: handle -EINTR in cifs_setattr Ronnie Sahlberg
2020-10-06 10:56 ` Aurélien Aptel
2020-10-06 22:22 ` ronnie sahlberg
2020-10-07 1:44 ` Steve French
2020-10-07 2:48 ` Steve French
2020-10-07 10:45 ` Aurélien Aptel
2020-10-08 23:31 ` ronnie sahlberg
-- strict thread matches above, loose matches on Subject: below --
2020-10-08 23:32 Ronnie Sahlberg
2020-10-09 5:43 ` Steve French
2020-10-06 2:26 Ronnie Sahlberg
2020-10-06 4:32 ` Steve French
2020-10-06 5:06 ` Shyam Prasad N
2020-10-06 5:09 ` Shyam Prasad N
2020-10-06 5:23 ` ronnie sahlberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.