Message-Id: <1211234829.3663.39.camel@localhost.localdomain>
Date:	Mon, 19 May 2008 15:07:09 -0700
From:	Mingming Cao <cmm@...ibm.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	jack@...e.cz, pbadari@...ibm.com, linux-ext4@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [PATCH] JBD: Fix DIO EIO error caused by race between free
	buffer and commit trasanction

On Mon, 2008-05-19 at 13:25 -0700, Andrew Morton wrote:
> On Mon, 19 May 2008 12:59:18 -0700
> Mingming Cao <cmm@...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@...e.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".
> 

This make sense to me.

> 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.
> 
Sure. We could check __GFP_FS and __GFP_WAIT, and that make sense.

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

Yes, it looks like page_cache_pipe_buf_steal() expects page is free
before removeing it by passing the GFP_KERNEL flag, but currently ext3
could fails to releasepage when it called. In fact try_to_release_page()
return value is ignored in page_cache_pipe_buf_steal(), should probably
checked the failure case.


The other caller of try_to_release_page() in mm/splice.c is
fallback_migrate_page(), which does want the synchronous behaviour to
make sure buffers are dropped.


I will reuse the GFP_WAIT and GFP_FS flag in the updated patch.

Thanks for your feedback.

Mingming
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists