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@xxxxxxx>
SUSE Labs, CR
--
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
