On Sat, Mar 02, 2013 at 11:32:07PM +0200, Alex Lyakas wrote: > On Fri, Mar 1, 2013 at 6:17 PM, David Sterba <dste...@suse.cz> wrote: > > - btrfs_kill_all_delayed_nodes(root); > > + btrfs_kill_all_delayed_nodes(root); > > > > - if (btrfs_header_backref_rev(root->node) < > > - BTRFS_MIXED_BACKREF_REV) > > - ret = btrfs_drop_snapshot(root, NULL, 0, 0); > > - else > > - ret =btrfs_drop_snapshot(root, NULL, 1, 0); > > - BUG_ON(ret < 0); > > - } > > - return 0; > > + if (btrfs_header_backref_rev(root->node) < > > + BTRFS_MIXED_BACKREF_REV) > > + ret = btrfs_drop_snapshot(root, NULL, 0, 0); > > + else > > + ret = btrfs_drop_snapshot(root, NULL, 1, 0); > > + /* > > + * If we encounter a transaction abort during snapshot cleaning, we > > + * don't want to crash here > > + */ > > + BUG_ON(ret < 0 && (ret != -EAGAIN || ret != -EROFS)); > > + return run_again || ret == -EAGAIN; > I think that this expression will always evaluate to "true", because > "run_again" is set to 1 and never reset.
That's right, I probably had some intentions in the past, but now it's just superflous. > So now if btrfs_drop_snapshot() returns -EAGAIN, then > btrfs_clean_one_deleted_snapshot() will return 1, thus telling the > caller "call me again", like your comment says. However, in this case > we are unmounting, so is this ok? Or this is just to avoid the cleaner > going to sleep? close_ctree calls kthread_stop(cleaner) which would wake cleaner anyway. The cases when we want send cleaner to sleep are when there's nothing to do or the read-only case which is just a safety check. >From that point, the last statement should be 'return 1' and run_again removed. > Also, I want to ask, hope this is not inappropriate. Do you also agree > with Josef, that it's ok for BTRFS_IOC_SNAP_DESTROY not to commit the > transaction, but just to detach from it? Had we committed, we would > have ensured that ORPHAN_ITEM is in the root tree, thus preventing > from subvol to re-appear after crash. > It seems a little inconsistent with snap creation, where not only the > transaction is committed, but delalloc flush is performed to ensure > that all data is on disk before creating the snap. That's another question, can you please point me to the thread where this was discussed? thanks, david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
