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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
