Re: [PATCH 7/9] fsfreeze: freeze_super and thaw_bdev don't play well together

[Jan Kara <jack@suse.cz>]

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@suse.cz>
SUSE Labs, CR