On Wed, Sep 16, 2009 at 04:09:13PM +0200, Jan Kara wrote:
> On Tue 15-09-09 14:39:06, Chris Mason wrote:
[ ... ]
> > > 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.
Sorry, guarded in ext3 is ordered in btrfs and I'm easily confused.
>
> > > 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.
Fair enough, I plan on hammering out the next version today or tomorrow.
-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
