Re: [PATCH 2/2] Ext3: data=guarded mode

[Jan Kara <jack@suse.cz>]

On Tue 15-09-09 14:39:06, Chris Mason wrote:
> On Tue, Sep 15, 2009 at 07:29:24PM +0200, Jan Kara wrote:
> > > +static int orphan_del(handle_t *handle, struct inode *inode, int must_log)
> > > +{
> > > +	int ret = 0;
> > > +	struct list_head *ordered_list;
> > > +
> > > +	ordered_list = &EXT3_I(inode)->ordered_buffers.ordered_list;
> > > +
> > > +	/* fast out when data=guarded isn't on */
> > > +	if (!ext3_should_guard_data(inode)) {
> > > +		WARN_ON(must_log);
> > > +		return ext3_orphan_del(handle, inode);
> > > +	}
> > > +
> > > +	ext3_ordered_lock(inode);
> > > +	if (inode->i_nlink && list_empty(ordered_list)) {
> > > +		ext3_ordered_unlock(inode);
> > > +
> > > +		lock_super(inode->i_sb);
> > > +
> > > +		/*
> > > +		 * now that we have the lock make sure we are allowed to
> > > +		 * get rid of the orphan.  This way we make sure our
> > > +		 * test isn't happening concurrently with someone else
> > > +		 * adding an orphan.  Memory barrier for the ordered list.
> > > +		 */
> > > +		smp_mb();
> > > +		if (inode->i_nlink == 0 || !list_empty(ordered_list)) {
> >   The code here still looks suspicious.
> > 1) Inodes can be on orphan list with i_nlink > 0 when a write failed for
> >    some reason and we have to truncate blocks instantiated beyond i_size.
> >    Those places (similarly as truncate) expect that while they hold i_mutex
> >    they are safe doing what they want with the orphan list. This code would
> >    happily remove the inode from orphan list...
> 
> The only risky place for this is the work thread doing the ordered
> writes.  Truncate gets around it by waiting for the ordered completions.
> I'll add the wait to the error handlers as well.
  You probably mean guarded writes. I agree.

> > 2) Cannot it happen that:
> >      CPU1
> > orphan_del()
> >   if (inode->i_nlink && list_empty(ordered_list)) {
> > 	ext3_ordered_unlock(inode);
> > 	lock_super(inode->i_sb);
> > 	smp_mb();
> > 	if (inode->i_nlink == 0 || !list_empty(ordered_list)) {
> > 
> >      CPU2
> > journal_dirty_data_guarded_fn()
> >   ret = ext3_add_ordered_extent(inode, offset, bh);
> >   if (ret == 0 && buffer_dataguarded(bh) &&
> >       list_empty(&EXT3_I(inode)->i_orphan) &&
> >       !(EXT3_SB(inode->i_sb)->s_mount_state & EXT3_ORPHAN_FS)) - list isn't
> > empty yet so we don't add inode to orphan list, but on CPU1, we go ahead
> > and remove inode from the orphan list...
> 
> This used to have a check after the orphan_del to re-add the orphan if
> we raced with the end_io handlers.  I removed it because I thought it
> was over-paranoid, but I see that you're right.  So, I'll put that one
> back in.
  Hmm, that will probably work but it's ugly :(. The ugliness is localized
in the guarded mode code so probably we can bear it for a while but I'll
certainly try to look into what we can do to get rid of it :).

> > > diff --git a/include/linux/ext3_fs_i.h b/include/linux/ext3_fs_i.h
> > > index ca1bfe9..a6cf26d 100644
> > > --- a/include/linux/ext3_fs_i.h
> > > +++ b/include/linux/ext3_fs_i.h
> > > @@ -137,6 +180,8 @@ struct ext3_inode_info {
> > >  	 * by other means, so we have truncate_mutex.
> > >  	 */
> > >  	struct mutex truncate_mutex;
> > > +
> > > +	struct ext3_ordered_buffers ordered_buffers;
> > >  	struct inode vfs_inode;
> > >  };
> >   Hmm, how hard would it be to hide especially this behind
> > CONFIG_EXT3_GUARDED_DATA so that we can avoid increasing inode size for
> > users which are not interested in the new guarded mode?
> 
> I'm not too picky, but it would litter the code with #ifdefs around the
> guarded functions.  I'd rather not.
  Looking into the code, it needn't be too bad if we define a a few
functions as empty in !guarded case. I'll have a look at it for the next
version of your patch.

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