diff mbox

[linux-cifs-client] cifs: Detect errors in cifs_writepages when called from pdflush

Message ID 20090417124438.GB31321@kay (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Schwenke April 17, 2009, 12:44 p.m. UTC
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(-)

Comments

Peter Schwenke April 17, 2009, 1:40 p.m. UTC | #1
I forgot to mention that this is on the 2.6.27 tree.
Jeff Layton April 17, 2009, 3:50 p.m. UTC | #2
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,
Peter Schwenke April 20, 2009, 7:35 a.m. UTC | #3
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 mbox

Patch

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);