[PATCH] O_SYNC error handling fix

[PATCH] O_SYNC error handling fix

[Chris Mason]

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

Re: [PATCH] O_SYNC error handling fix

[Andrew Morton]

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?

Re: [PATCH] O_SYNC error handling fix

[Chris Mason]

On Monday 16 January 2006 02:58, Andrew Morton wrote:
> 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.
[ ... ]

>
> 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.

True, I can get rid of the O_DIRECT change, it was just to preserve the status 
variable for the -EIO test.

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

Skipping the file position update was because we are not returning the short 
write.  We want the next write to happen at the appropriate place given the 
number of bytes we've told userland went out.

>
> 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?

With O_SYNC we've no way of knowing which bytes didn't make it down to the 
disk.  I can special case the -EIO check for just O_SYNC. I think that along 
with changing around the O_DIRECT bits will make us both happy?

-chris

Re: [PATCH] O_SYNC error handling fix

[Andrew Morton]

Chris Mason <mason@suse.com> wrote:
>
> I can special case the -EIO check for just O_SYNC. I think that along 
>  with changing around the O_DIRECT bits will make us both happy?

I guess so...

It seems I'm full of crap anyway.  We already return -EFOO on failing O_SYNC
writes, no?