Re: [PATCH] Fix ext2 error reporting on fsync

[Andrew Morton <akpm@osdl.org>]

Jan Kara <jack@suse.cz> wrote:
>
> > Jan Kara <jack@suse.cz> wrote:
> > >
>   <snip>
> > > +	 */
> > > +	if (test_and_clear_bit(AS_EIO, &sb->s_bdev->bd_inode->i_mapping->flags)) {
> > > +		ext2_error(sb, "ext2_sync_file", "metadata io error");
> > > +		if (!ret)
> > > +			ret = -EIO;
> > > +	}
> > >  	if (!(inode->i_state & I_DIRTY))
> > >  		return ret;
> > >  	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
> > > 
> > 
> > This is desperation stuff, isn't it?
> > 
> > It would be better to stick a BH_WriteError into buffer_head.b_state, set
> > that in the I/O completion handler, check it in fsync_buffers_list()?
>   It actually depends on the desired behaviour of fsync() in case of
> IO errors. Consider the following scenario:
>   * process writes some data
>     - async writing starts in background and gets an IO error on some
>       metadata buffer
>   * process calls fsync()
>     - all the remaining buffers are written without problems
> 
>   If I understand your solution correctly, it need not report any error
> in this case as buffer with IO error could be already removed from
> mapping->private_list by try_to_free_buffers(). But it would be nice to
> report to the user that we were not able to write all the data...
> 

hm, yes, try_to_free_buffers() against a blockdev page is a problem.  But I
think it's fixable.

In the second loop of drop_buffers(), if we find a bh which had a write
error we need to set AS_EIO on the address_space which is interested in
this buffer.

That address_space is pinned by a) the fact that it has buffers at
->private_list and b) we hold the blockdev mapping's private_lock.

The only problem is actually _finding_ the address_space which is
interested in this buffer_head.  Looks like we'd need a backpointer in the
buffer_head.