Re: [PATCH] JBD: Fix DIO EIO error caused by race between free buffer and commit trasanction

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 19 May 2008 12:59:18 -0700
Mingming Cao <cmm@us.ibm.com> wrote:

> On Mon, 2008-05-19 at 00:37 +0200, Jan Kara wrote:
> >   Hi,
> > 
> > > This patch fixed a few races between direct IO and kjournald commit
> > > transaction.  An unexpected EIO error gets returned to direct IO
> > > caller when it failed to free those data buffers. This could be
> > > reproduced easily with parallel direct write and buffered write to the
> > > same file
> > > 
> > > More specific, those races could cause journal_try_to_free_buffers()
> > > fail to free the data buffers, when jbd is committing the transaction
> > > that has those data buffers on its t_syncdata_list or t_locked_list.
> > > journal_commit_transaction() still holds the reference to those
> > > buffers before data reach to disk and buffers are removed from the
> > > t_syncdata_list of t_locked_list. This prevent the concurrent
> > > journal_try_to_free_buffers() to free those buffers at the same time,
> > > but cause EIO error returns back to direct IO.
> > > 
> > > With this patch, in case of direct IO and when try_to_free_buffers() failed,
> > > let's waiting for journal_commit_transaction() to finish
> > > flushing the current committing transaction's data buffers to disk, 
> > > then try to free those buffers again.
> >   If Andrew or Christoph wouldn't beat you for "inventive use" of
> > gfp_mask, I'm fine with the patch as well ;). You can add
> >   Acked-by: Jan Kara <jack@suse.cz>
> > 
> 
> This is less intrusive way to fix this problem. The gfp_mask was marked
> as unused in try_to_free_page(). I looked at filesystems in the kernel,
> there is only a few defined releasepage() callback, and only xfs checks
> the flag(but not used). btrfs is actually using it though. I thought
> about the way you have suggested, i.e.clean up this gfp_mask and and
> replace with a flag.  I am not entirely sure if it we need to change the
> address_space_operations and fix all the filesystems for this matter.
> 
> Andrew, what do you think? Is this approach acceptable? 
> 

<wakes up>

Please ensure that the final patch is sufficiently well changelogged to
permit me to remain asleep ;)

The ->releasepage semantics are fairly ad-hoc and have grown over time.
It'd be nice to prevent them from becoming vaguer than they are.

It has been (approximately?) the case that code paths which really care
about having the page released will set __GFP_WAIT (via GFP_KERNEL)
whereas code paths which are happy with best-effort will clear
__GFP_WAIT (with a "0').  And that's reasonsable - __GFP_WAIT here
means "be synchronous" whereas !__GFP_WAIT means "be non-blocking".

Is that old convention not sufficient here as well?  Two problem areas
I see are mm/vmscan.c and fs/splice.c (there may be others).

In mm/vmscan.c we probably don't want your new synchronous behaviour
and it might well be deadlockable anyway.  No probs, that's what
__GFP_FS is for.

In fs/splice.c, reading the comment there I have a feeling that you've
found another bug, and that splice _does_ want your new synchronous
behaviour?