Re: [Ocfs2-devel] [PATCH 01/18] jbd2: Add buffer triggers

[Jan Kara <jack@suse.cz>]

On Thu 18-12-08 16:32:25, Joel Becker wrote:
> On Thu, Dec 18, 2008 at 07:08:21PM +0100, Jan Kara wrote:
> > > On Tue, Dec 09, 2008 at 05:09:38PM -0800, Joel Becker wrote:
> > > > Filesystems often to do compute intensive operation on some
> > > > metadata.  If this operation is repeated many times, it can be very
> > > > expensive.  It would be much nicer if the operation could be performed
> > > > once before a buffer goes to disk.
> > > 
> > > 	I realized, well, that I'm an idiot.  The previous patch has a
> > > significant bug: what if a block is deallocated, then reused as a
> > > different type of metadata, all before the committing transaction gets
> > > around to firing the triggers?  It could use the new block type's
> > > triggers against the b_frozen_data of the old block type.
> > > 	The easy answer is to add b_frozen_triggers alongside
> > > b_frozen_data.  Here's the new patch.
> >   I think this is a reasonable thing to do, although I'm not sure it can
> > really happen - at least ext3 uses a mechanism that does not allow
> > reusing a freed block in the same transaction (because otherwise there's
> > no way of recovering unjournaled data after crash).
> 
> 	Oh, frozen_data protects that sort of thing.  The concern is
> that the old block type is in the committing transaction, and the new
> block type is in the running transaction.  But the trigger type is from
> the new block type, and not valid to call against the committing block
> in the committing transaction.
  Sorry, I've confused frozen_data with committed_data. Buffer with
frozen_data happens quite often. So your fix is really needed.

> > > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > > index ebc667b..c8a1bac 100644
> > > --- a/fs/jbd2/commit.c
> > > +++ b/fs/jbd2/commit.c
> > > @@ -509,6 +509,10 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> > >  		if (is_journal_aborted(journal)) {
> > >  			clear_buffer_jbddirty(jh2bh(jh));
> > >  			JBUFFER_TRACE(jh, "journal is aborting: refile");
> > > +			jbd2_buffer_abort_trigger(jh,
> > > +						  jh->b_frozen_data ?
> > > +						  jh->b_frozen_triggers :
> > > +						  jh->b_triggers);
> >   Wouldn't it be nicer if the jbd2_buffer_foo_trigger() functions
> > checked which set of triggers they should use and used it?
> 
> 	Well, it only does on the frozen-data check.  I suppose that
> could be pulled into the abort_trigger() function.  Maybe an additional
> patch?
  Yeah, I was just wondering why we don't do:
jbd2_buffer_abort_trigger(jh)
  and choose the proper set of triggers in the jbd2_buffer_abort_trigger()
function (and similarly for the commit trigger). Or does it make sence
to not call frozen trigger in some place if there're frozen data set?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR