From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Morton <akpm@osdl.org>
Subject: Re: [PATCH] O_SYNC error handling fix
Date: Sun, 15 Jan 2006 23:58:52 -0800
Message-ID: <20060115235852.36a8bcad.akpm@osdl.org>
References: <200601151943.51771.mason@suse.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: linux-fsdevel@vger.kernel.org
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from smtp.osdl.org ([65.172.181.4]:33420 "EHLO smtp.osdl.org")
	by vger.kernel.org with ESMTP id S1750917AbWAPH7O (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 16 Jan 2006 02:59:14 -0500
To: Chris Mason <mason@suse.com>
In-Reply-To: <200601151943.51771.mason@suse.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-Id: linux-fsdevel.vger.kernel.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?