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