On Fri, Jun 21, 2013 at 08:17:09PM +0800, zwu.kernel@xxxxxxxxx wrote: > From: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx> > > The patchset is trying to introduce hot tracking function in > VFS layer, which will keep track of real disk I/O in memory. > By it, you will easily know more details about disk I/O, and > then detect where disk I/O hot spots are. Also, specific FS > can take use of it to do accurate defragment, and hot relocation > support, etc. > > After V1 was sent out, Chandra Seetharaman has reviewed and > made a lot of comments, thanks a lot to him. Now it's time to > send out its V3 for external review, any comments or ideas are > appreciated, thanks. First of all, my apologies for obscenely long delay with review. I've started doing it several times and dropped these attempts getting mired in the nightmare of lifetime rules in there. What I should've done was sending the obvious low-hanging fruits right then and waiting for the variant with that stuff sanitized ;-/ First of all, one general observation: please, separate inode and range stuff clearly; do *not* use the same functions on both, it only makes harder to read. I.e. kill hot_comm_item and split the functions that take it. This code is trying to be too generic for its own good and ends up being obfuscated to hell and back... refcounting: * the whole refcount + "DELETING" flag approach is a bad idea. hot_comm_item_unlink() tries to be idempotent *and* includes dropping the reference on its first call. Schemes like that tend to be either pointless (i.e. we know that it won't be called twice for the same object) or prone to stepping on dangling pointers. This one does the latter (at least). * lookups (both for inode and range) leak on race with unlink. You grab a reference, drop the lock, check if HOT_DELETING is set and return an error if it is; how the hell could the caller possibly undo that reference grab, when it has no idea what object had been grabbed in the first place? Moreover, that check does *not* prevent getting an object with HOT_DELETING from those functions - unlink coming just after that check will be unnoticed. hot_inode_item_delete() mess: * hot_inode_item_delete() is done on unlink(2), no matter how many links are there. Why? * hot_inode_item_delete() is done even if unlink() fails (e.g. with EBUSY, or on whatever error ->unlink() might return). * hot_inode_item_delete() grabs a reference to hot_inode_item, *drops* *it*, then does hot_comm_item_unlink(). What protects it from being freed right as we drop the damn reference? debugfs-related issues - debugfs is completely unsuitable for dynamic objects and you step into that big way, in addition to races of your own: * creation of hot_debugfs_root is racy - WTF prevents two hot_track_init() in parallel? * if creation fails, we leave hot_debugfs_root ERR_PTR(something); from that point on, no attempts to create it will be done (check for hot_debugfs_root being *NULL*) * what guarantees that sb->s_id is unique? * removal on failure - screwed; first of all, list_empty() will *not* be true if we have somebody open it (cursors are inserted into that list). Moreover, what's to stop another hot_debugfs_init() from being called just as we are doing debugfs_remove(hot_debugfs_root) and see that it's non-NULL *before* we get to assigning NULL there? * hot_debugfs_exit() - screwed in the same way. * debugfs files get ->i_private set to corresponding sb->s_hot_root. It's copied to seq->private on open() and used by iterators. WTF prevents open on debugfs, followed by umount of corresponding btrfs volume, freeing of sb->s_hot_root and then read() on our file stepping into kfree'd hot_root in ->start()? -- 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
