On Wed 26-09-12 17:22:53, Fernando Luis Vazquez Cao wrote:

On 2012/09/26 01:39, Jan Kara wrote:

quoted

quoted

quoted

quoted

@@ -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 [off-list ref]
SUSE Labs, CR