On Wed 26-09-12 17:22:53, Fernando Luis Vazquez Cao wrote:
> On 2012/09/26 01:39, Jan Kara wrote:
> >>>>@@ -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...
>
> I should have explained my fears better. If no one else
> is holding an active reference we will end up executing:
>
> fs->kill_sb(s);
> ...
> put_filesystem(fs);
> put_super(s);
>
> in deactivate_locked_super(). If s_count is greater than 0
> the superblock will not be freed, as you say, however we
> still do "fs->kill_sb(s)" and "put_filesystem(fs)" and I am not
> sure whether this is safe when MS_BORN flag is not set in
> the superblock. I will check how MS_BORN is being used
> once more and try to figure it out (if you are familiar with
> MS_BORN I would appreciate it your advise).
I see. Well, from a brief check I don't think we should ever get to a
superblock without MS_BORN set. All functions iterating over superblocks
return only superblocks with MS_BORN set. In particular freeze_bdev() even
has an active reference itself and ioctl_fsfreeze() has a file open on the
sb which guarantees an active reference as well...
So maybe just removing that check altogether should be OK.
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
