On Tue 25-09-12 19:51:33, Fernando Luis Vazquez Cao wrote:
> On 2012/09/25 18:48, Jan Kara wrote:
> >On Fri 14-09-12 15:53:34, Fernando Luis Vázquez Cao wrote:
> >>thaw_bdev() has re-entrancy guards to allow freezes to nest
> >>together. That is, it ensures that the filesystem is not thawed
> >>until the last thaw command is issued. This is needed to prevent the
> >>filesystem from being unfrozen while an existing freezer is still
> >>operating on the filesystem in a frozen state (e.g. dm-snapshot).
> >>
> >>Currently, freeze_super() and thaw_super() bypasses these guards,
> >>and as a result manual freezing and unfreezing via the ioctl methods
> >>do not nest correctly. hence mixing userspace directed freezes with
> >>block device level freezes result in inconsistency due to premature
> >>thawing of the filesystem.
> >>
> >>Move the re-enterency guards to the superblock and into freeze_super
> >>and thaw_super() so that userspace directed freezes nest correctly
> >>again. Caveat: Things work as expected as long as direct calls to
> >>thaw_super are always in response to a previous sb level freeze. In
> >>other words an unpaired call to thaw_super can still thaw a
> >>filesystem frozen using freeze_bdev (this issue could be addressed
> >>in a follow-up patch if deemed necessary).
> >>
> >>This patch retains the bdev level mutex and counter to keep the
> >>"feature" that we can freeze a block device that does not have a
> >>filesystem mounted yet.
> >>
> >>IMPORTANT CAVEAT: This patch restores the nesting feature of the fsfreeze ioctl
> >>which got removed by commit 18e9e5104fcd9a973ffe3eed3816c87f2a1b6cd2
> >>("Introduce freeze_super and thaw_super for the fsfreeze ioctl"). It could be
> >>argued that it is too late to fix the userland ABI breakage caused by that
> >>patch and that the current ABI is the one that should be kept. If this is the
> >>respective maintainer(s) opinion this could be modified to not allow fsfreeze
> >>ioctl nesting.
> >>
> >>Changes since version 2:
> >> - Fix reference count leak in freeze_super when MS_BORN flag is not set in
> >> the superblock.
> >> - Protect freeze counter using s_umount and get rid of superblock level
> >> fsfreeze mutex.
> >>
> >>Cc: Josef Bacik <jbacik@xxxxxxxxxxxx>
> >>Cc: Eric Sandeen <sandeen@xxxxxxxxxx>
> >>Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> >>Cc: Jan Kara <jack@xxxxxxx>
> >>Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> >>Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
> >>---
> > I have just some mostly minor comments:
>
> Thank you for the detailed review.
>
>
> >>diff -urNp linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c linux-3.6-rc5/fs/gfs2/ops_fstype.c
> >>--- linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c 2012-09-14 12:46:15.152871285 +0900
> >>+++ linux-3.6-rc5/fs/gfs2/ops_fstype.c 2012-09-14 13:51:36.457205813 +0900
> >>@@ -1288,11 +1288,6 @@ static struct dentry *gfs2_mount(struct
> >> if (IS_ERR(bdev))
> >> return ERR_CAST(bdev);
> >>- /*
> >>- * once the super is inserted into the list by sget, s_umount
> >>- * will protect the lockfs code from trying to start a snapshot
> >>- * while we are mounting
> >>- */
> > Shouldn't this comment be replaced with something more accurate instead
> >of just deleting it?
>
> Good point.
>
>
> >>@@ -1365,29 +1363,27 @@ static void sb_wait_write(struct super_b
> >> * freezing. Then we transition to SB_FREEZE_COMPLETE state. This state is
> >> * mostly auxiliary for filesystems to verify they do not modify frozen fs.
> >> *
> >>- * sb->s_writers.frozen is protected by sb->s_umount.
> >>+ * sb->s_writers.frozen and sb->s_freeze_count are protected by sb->s_umount.
> >> */
> >> int freeze_super(struct super_block *sb)
> >> {
> >>- int ret;
> >>+ int ret = 0;
> >> atomic_inc(&sb->s_active);
> >> down_write(&sb->s_umount);
> >>- if (sb->s_writers.frozen != SB_UNFROZEN) {
> >>- deactivate_locked_super(sb);
> >>- return -EBUSY;
> >>- }
> >> if (!(sb->s_flags & MS_BORN)) {
> >>- up_write(&sb->s_umount);
> >>- return 0; /* sic - it's "nothing to do" */
> >>+ atomic_dec(&sb->s_active);
> >>+ goto out_active; /* sic - it's "nothing to do" */
> > Why not 'goto out_deactivate' here instead of manually decrementing
> >s_active?
>
> I was afraid that calling deactivate_locked_super()
> when the MS_BORN flag is set could release the
> last active reference to the superblock and
> end up freeing it.
Well, the caller has to have the sb pinned somehow so that it can safely
pass it into freeze_super(). So deactivate_locked_super() cannot really
end up freeing the superblock...
> By the way, I probably should explain in the
> patch that that piece of code fixes a reference
> leak bug in the current code.
Yep, that would be nice.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
