Re: [RFC][PATCH]btrfs delete ordered inode handling fix

[Chris Mason <chris.mason@oracle.com>]

On Thursday 22 May 2008, Mingming wrote:
> On Thu, 2008-05-22 at 10:11 -0400, Chris Mason wrote:
> > On Wednesday 21 May 2008, Mingming wrote:
> > > Hi Chris,  I thought I spotted a few bugs, While looking at how to
> > > properly remove inode from ordered tree, let me know if I got it right.
> >
> > Hi Mingming, thanks for going through this code.  The i_count rules in
> > the current code should work like this:
>
> Hi Chris, thanks for your detailed clarification.
>
> > * btrfs_add_ordered_inode calls igrab when the inode is inserted into the
> > list.  The whole time the inode is on the list, there's an extra
> > reference on i_count.  There will be no final iput while the inode is on
> > the list.
>
> Ah I missed that. That explains all my confusion of the i_count accounting.
>
> > > * There is possible race with inode delete and
> > > btrfs_find_first_ordered_inode(). The inode could possibly in the
> > > process of freeing while we are trying to get hold of it during commit
> > > transaction. The fix is using igrab() instead, and search for next
> > > inode in the tree if the found one is in the middle of being released.
> >
> > These kinds of races where the main reason why I had the list take a
> > reference on the inode.  delete_inode won't be called while i_count is
> > increased.
> >
> > Over the long term I'd prefer to move the ordered-data list to a model
> > where the list doesn't have a reference and it is magically removed after
> > all the dirty pages are gone (by the end_io_hook handlers in inode.c). 
> > The end_io hooks in inode.c may be sufficient for this.
>
> Make sense.
>
> > > * get rid of btrfs_put_inode(), and move the functionality under the
> > > btrfs_del_ordered_inode() directly.
> >
> > I like this change, thanks.
> >
> > > * Remove the inode from ordered tree at last iput(). Did not do it at
> > > file release() time, as it may remove the inode from the ordered tree
> > > before ensure the ordering of write to the same inode from other
> > > process.
> > >
> > > Perhaps calling btrfs_del_ordered_inode() under unlink() is enough, but
> > > it would not be hurt to do it again at delete_inode() time.
> >
> > I'm afraid we'll have to do it at file_release time, at least until the
> > ordered list is changed not to keep a reference.
>
> Yes with the i_count logic delete_inode() is not the right place to call
> btrfs_del_ordered_inode.
>
> But I am still not quite sure whether it is safe to remove the inode
> from the ordered tree at the file_release() time. i.e. whether the dirty
> data already being flushed to disk at last file_close()/file_release()
> time and when two process open and write to the same inode ...

I get around this by testing for dirty/writeback pages before removing the 
inode from the ordered list.  If another writer allocates blocks to the file, 
it will be added back to the list.

-chris