Re: [PATCH] O_SYNC error handling fix

[Andrew Morton <akpm@osdl.org>]

Chris Mason <mason@suse.com> wrote:
>
> 
> If we hit errors during generic_osync_inode(), generic_file_buffered_write 
> will still return written ? written : status;  This patch ensures the -EIO 
> gets back up to userland at all times.
> 
> diff -r 7e48cbfbccad mm/filemap.c
> --- a/mm/filemap.c	Sat Jan 14 01:12:21 2006 +0800
> +++ b/mm/filemap.c	Fri Jan 13 13:48:02 2006 -0500
> @@ -2026,7 +2026,6 @@ generic_file_buffered_write(struct kiocb
>  		balance_dirty_pages_ratelimited(mapping);
>  		cond_resched();
>  	} while (count);
> -	*ppos = pos;
>  
>  	if (cached_page)
>  		page_cache_release(cached_page);
> @@ -2047,10 +2046,24 @@ generic_file_buffered_write(struct kiocb
>  	 * to buffered writes (block instantiation inside i_size).  So we sync
>  	 * the file data here, to try to honour O_DIRECT expectations.
>  	 */
> -	if (unlikely(file->f_flags & O_DIRECT) && written)
> +	if (unlikely(file->f_flags & O_DIRECT) && status >= 0 && written)
>  		status = filemap_write_and_wait(mapping);
>  
>  	pagevec_lru_add(&lru_pvec);
> +
> +	/*
> +	 * We must let know userspace if something hasn't been written
> +	 * correctly. If we got an I/O error it means we got an hardware
> +	 * failure, anything can be happening to the on-disk data,
> +	 * letting know userspace that a bit of data might have been
> +	 * written correctly on disk is a very low priority, compared
> +	 * to letting know userspace that some data has _not_ been
> +	 * written at all.
> +	 */
> +	if (unlikely(status == -EIO))
> +		return status;
> +	*ppos = pos;
> +
>  	return written ? written : status;
>  }
>  EXPORT_SYMBOL(generic_file_buffered_write);

The patch does more than this.  It also changes the O_DIRECT logic and it
also refuses to update the file position on I/O errors.

Probably reasonable changes, but I'd like to have your description of why
these changes were made please.


The patch also makes write() non-linuxy.  We normally return a short write
on errors.  In this case I'd say that returning 0 (and not updating f_pos)
would be appropriate?