Re: [PATCH] cifs: Detect errors in cifs_writepages when called from pdflush (V2)

On Mon, 20 Apr 2009 17:42:21 +1000
Peter Schwenke <[email protected]> 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 <[email protected]>
> ---
>  fs/cifs/file.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 042b122..c00a674 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1200,6 +1200,7 @@ static int cifs_writepages(struct address_space *mapping,
>  	unsigned int bytes_to_write;
>  	unsigned int bytes_written;
>  	struct cifs_sb_info *cifs_sb;
> +	struct cifsInodeInfo *cifs_inode = CIFS_I(mapping->host);
>  	int done = 0;
>  	pgoff_t end;
>  	pgoff_t index;
> @@ -1354,7 +1355,7 @@ retry:
>  			 * CIFSSMBWrite2.  We can't rely on the last handle
>  			 * we used to still be valid
>  			 */
> -			open_file = find_writable_file(CIFS_I(mapping->host));
> +			open_file = find_writable_file(cifs_inode);
>  			if (!open_file) {
>  				cERROR(1, ("No writable handles for inode"));
>  				rc = -EBADF;
> @@ -1374,6 +1375,18 @@ 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_inode->write_behind_rc =  rc;
> +						else
> +							cifs_inode->write_behind_rc =  -EIO;
> +					}
>  				} else {
>  					cifs_stats_bytes_written(cifs_sb->tcon,
>  								 bytes_written);

Looking at this a little more closely...

I'm not sure that current_is_pdflush() the right check to use for this?
Is it possible that we could get similar problem within the context of
another thread? Should we instead check for whether writepages was
called with WB_SYNC_NONE instead?

-- 
Jeff Layton <[email protected]>