| Message ID | 20090417124438.GB31321@kay (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I forgot to mention that this is on the 2.6.27 tree.
On Fri, 17 Apr 2009 22:44:38 +1000 Peter Schwenke <peter@bluetoad.com.au> wrote: > > The return code isn't checked in the case of cifs_writepages being > invoked as a result of pdflush. This results in data integrity > problems when errors such as a network break occur when > CIFSSMBWrite2 is called. The resulting EAGAIN isn't acted > on. > > Signed-off-by: Peter Schwenke <peter@bluetoad.com.au> > --- > fs/cifs/file.c | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 042b122..8cc1094 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -1374,6 +1374,17 @@ retry: > set_bit(AS_ENOSPC, &mapping->flags); > else > set_bit(AS_EIO, &mapping->flags); > + /* > + * The return code isn't checked in the > + * case of cifs_writepages being > + * invoked as a result of pdflush. > + * See generic_sync_sb_inodes() > + */ > + if (current_is_pdflush()) > + if (rc == -ENOSPC) > + CIFS_I(mapping->host)->write_behind_rc = rc; > + else > + CIFS_I(mapping->host)->write_behind_rc = -EIO; > } else { > cifs_stats_bytes_written(cifs_sb->tcon, > bytes_written); Looks reasonable, but doesn't the compiler complain about ambiguous else's here? You might need to add some curly braces here. Also, we should already have a 'cifsi' variable in that function that's usable instead of having to call CIFS_I. Thanks,
Jeff Layton wrote: > On Fri, 17 Apr 2009 22:44:38 +1000 > Peter Schwenke <peter@bluetoad.com.au> wrote: > >> @@ -1374,6 +1374,17 @@ retry: >> set_bit(AS_ENOSPC, &mapping->flags); >> else >> set_bit(AS_EIO, &mapping->flags); >> + /* >> + * The return code isn't checked in the >> + * case of cifs_writepages being >> + * invoked as a result of pdflush. >> + * See generic_sync_sb_inodes() >> + */ >> + if (current_is_pdflush()) >> + if (rc == -ENOSPC) >> + CIFS_I(mapping->host)->write_behind_rc = rc; >> + else >> + CIFS_I(mapping->host)->write_behind_rc = -EIO; >> } else { >> cifs_stats_bytes_written(cifs_sb->tcon, >> bytes_written); > > Looks reasonable, but doesn't the compiler complain about ambiguous > else's here? You might need to add some curly braces here. Also, we should > already have a 'cifsi' variable in that function that's usable instead > of having to call CIFS_I. > Oops. Sorry about that, I missed the warning. I've add the braces. There wasn't a variable like that available, so I've added one named cifs_inode (looked at one of Steve's latest patches and through the code to decide on a naming convention. I ummed and aaghed about using the variable in another spot where CIFS_I was used and went with it. Coming in next message.
diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 042b122..8cc1094 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -1374,6 +1374,17 @@ retry: set_bit(AS_ENOSPC, &mapping->flags); else set_bit(AS_EIO, &mapping->flags); + /* + * The return code isn't checked in the + * case of cifs_writepages being + * invoked as a result of pdflush. + * See generic_sync_sb_inodes() + */ + if (current_is_pdflush()) + if (rc == -ENOSPC) + CIFS_I(mapping->host)->write_behind_rc = rc; + else + CIFS_I(mapping->host)->write_behind_rc = -EIO; } else { cifs_stats_bytes_written(cifs_sb->tcon, bytes_written);
The return code isn't checked in the case of cifs_writepages being invoked as a result of pdflush. This results in data integrity problems when errors such as a network break occur when CIFSSMBWrite2 is called. The resulting EAGAIN isn't acted on. Signed-off-by: Peter Schwenke <peter@bluetoad.com.au> --- fs/cifs/file.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)