[PATCH 1/3] Implement generic freeze feature

[PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

The ioctls for the generic freeze feature are below.
o Freeze the filesystem
  int ioctl(int fd, int FIFREEZE, arg)
    fd: The file descriptor of the mountpoint
    FIFREEZE: request code for the freeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

o Unfreeze the filesystem
  int ioctl(int fd, int FITHAW, arg)
    fd: The file descriptor of the mountpoint
    FITHAW: request code for unfreeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

Signed-off-by: Takashi Sato <t-sato@yk.jp.nec.com>
Signed-off-by: Masayuki Hamaguchi <m-hamaguchi@ys.jp.nec.com>
---
 fs/block_dev.c              |    2 +
 fs/buffer.c                 |   27 ++++++++++++++++++-
 fs/ioctl.c                  |   61 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/buffer_head.h |    2 -
 include/linux/fs.h          |    6 ++++
 5 files changed, 96 insertions(+), 2 deletions(-)

diff -uprN -X linux-2.6.27-rc2.org/Documentation/dontdiff linux-2.6.27-rc2.org/fs/block_dev.c linux-2.6.27-rc2-freeze/fs
/block_dev.c
--- linux-2.6.27-rc2.org/fs/block_dev.c	2008-08-06 13:49:54.000000000 +0900
+++ linux-2.6.27-rc2-freeze/fs/block_dev.c	2008-08-07 08:59:54.000000000 +0900
@@ -285,6 +285,8 @@ static void init_once(void *foo)
 	INIT_LIST_HEAD(&bdev->bd_holder_list);
 #endif
 	inode_init_once(&ei->vfs_inode);
+	/* Initialize semaphore for freeze. */
+	sema_init(&bdev->bd_freeze_sem, 1);
 }
 
 static inline void __bd_forget(struct inode *inode)
diff -uprN -X linux-2.6.27-rc2.org/Documentation/dontdiff linux-2.6.27-rc2.org/fs/buffer.c linux-2.6.27-rc2-freeze/fs/bu
ffer.c
--- linux-2.6.27-rc2.org/fs/buffer.c	2008-08-06 13:49:54.000000000 +0900
+++ linux-2.6.27-rc2-freeze/fs/buffer.c	2008-08-07 08:59:54.000000000 +0900
@@ -201,6 +201,15 @@ struct super_block *freeze_bdev(struct b
 {
 	struct super_block *sb;
 
+	down(&bdev->bd_freeze_sem);
+	bdev->bd_freeze_count++;
+	if (bdev->bd_freeze_count > 1) {
+		sb = get_super(bdev);
+		drop_super(sb);
+		up(&bdev->bd_freeze_sem);
+		return sb;
+	}
+
 	down(&bdev->bd_mount_sem);
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
@@ -219,6 +228,8 @@ struct super_block *freeze_bdev(struct b
 	}
 
 	sync_blockdev(bdev);
+	up(&bdev->bd_freeze_sem);
+
 	return sb;	/* thaw_bdev releases s->s_umount and bd_mount_sem */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -230,8 +241,20 @@ EXPORT_SYMBOL(freeze_bdev);
  *
  * Unlocks the filesystem and marks it writeable again after freeze_bdev().
  */
-void thaw_bdev(struct block_device *bdev, struct super_block *sb)
+int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
+
+	down(&bdev->bd_freeze_sem);
+	if (!bdev->bd_freeze_count) {
+		up(&bdev->bd_freeze_sem);
+		return 0;
+	}
+	bdev->bd_freeze_count--;
+	if (bdev->bd_freeze_count > 0) {
+		up(&bdev->bd_freeze_sem);
+		return 0;
+	}
+
 	if (sb) {
 		BUG_ON(sb->s_bdev != bdev);
 
@@ -244,6 +267,8 @@ void thaw_bdev(struct block_device *bdev
 	}
 
 	up(&bdev->bd_mount_sem);
+	up(&bdev->bd_freeze_sem);
+	return 0;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
diff -uprN -X linux-2.6.27-rc2.org/Documentation/dontdiff linux-2.6.27-rc2.org/fs/ioctl.c linux-2.6.27-rc2-freeze/fs/ioc
tl.c
--- linux-2.6.27-rc2.org/fs/ioctl.c	2008-08-06 13:49:54.000000000 +0900
+++ linux-2.6.27-rc2-freeze/fs/ioctl.c	2008-08-07 08:59:54.000000000 +0900
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/buffer_head.h>
 
 #include <asm/ioctls.h>
 
@@ -141,6 +142,57 @@ static int ioctl_fioasync(unsigned int f
 }
 
 /*
+ * ioctl_freeze - Freeze the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call freeze_bdev() to freeze the filesystem.
+ */
+static int ioctl_freeze(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support freeze feature, return. */
+	if (sb->s_op->write_super_lockfs == NULL)
+		return -EOPNOTSUPP;
+
+	/* If a regular file or a directory isn't specified, return. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	/* Freeze */
+	sb = freeze_bdev(sb->s_bdev);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+	return 0;
+}
+
+/*
+ * ioctl_thaw - Thaw the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call thaw_bdev() to thaw the filesystem.
+ */
+static int ioctl_thaw(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If a regular file or a directory isn't specified, return EINVAL. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	/* Thaw */
+	return thaw_bdev(sb->s_bdev, sb);
+}
+
+/*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
  *
@@ -181,6 +233,15 @@ int do_vfs_ioctl(struct file *filp, unsi
 		} else
 			error = -ENOTTY;
 		break;
+
+	case FIFREEZE:
+		error = ioctl_freeze(filp);
+		break;
+
+	case FITHAW:
+		error = ioctl_thaw(filp);
+		break;
+
 	default:
 		if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
 			error = file_ioctl(filp, cmd, arg);
diff -uprN -X linux-2.6.27-rc2.org/Documentation/dontdiff linux-2.6.27-rc2.org/include/linux/buffer_head.h linux-2.6.27-
rc2-freeze/include/linux/buffer_head.h
--- linux-2.6.27-rc2.org/include/linux/buffer_head.h	2008-08-06 13:49:54.000000000 +0900
+++ linux-2.6.27-rc2-freeze/include/linux/buffer_head.h	2008-08-07 08:59:54.000000000 +0900
@@ -170,7 +170,7 @@ void __wait_on_buffer(struct buffer_head
 wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 int fsync_bdev(struct block_device *);
 struct super_block *freeze_bdev(struct block_device *);
-void thaw_bdev(struct block_device *, struct super_block *);
+int thaw_bdev(struct block_device *, struct super_block *);
 int fsync_super(struct super_block *);
 int fsync_no_super(struct block_device *);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
diff -uprN -X linux-2.6.27-rc2.org/Documentation/dontdiff linux-2.6.27-rc2.org/include/linux/fs.h linux-2.6.27-rc2-freez
e/include/linux/fs.h
--- linux-2.6.27-rc2.org/include/linux/fs.h	2008-08-06 13:49:54.000000000 +0900
+++ linux-2.6.27-rc2-freeze/include/linux/fs.h	2008-08-07 08:59:54.000000000 +0900
@@ -226,6 +226,8 @@ extern int dir_notify_enable;
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
+#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
+#define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
@@ -574,6 +576,10 @@ struct block_device {
 	 * care to not mess up bd_private for that case.
 	 */
 	unsigned long		bd_private;
+	/* The counter of freeze processes */
+	int			bd_freeze_count;
+	/* Semaphore for freeze */
+	struct semaphore	bd_freeze_sem;
 };
 
 /*

Re: [PATCH 1/3] Implement generic freeze feature

[Andrew Morton]

On Mon, 18 Aug 2008 21:28:19 +0900
Takashi Sato <t-sato@yk.jp.nec.com> wrote:

> The ioctls for the generic freeze feature are below.
> o Freeze the filesystem
>   int ioctl(int fd, int FIFREEZE, arg)
>     fd: The file descriptor of the mountpoint
>     FIFREEZE: request code for the freeze
>     arg: Ignored
>     Return value: 0 if the operation succeeds. Otherwise, -1
> 
> o Unfreeze the filesystem
>   int ioctl(int fd, int FITHAW, arg)
>     fd: The file descriptor of the mountpoint
>     FITHAW: request code for unfreeze
>     arg: Ignored
>     Return value: 0 if the operation succeeds. Otherwise, -1
> 
>
> ...
>
> --- linux-2.6.27-rc2.org/include/linux/fs.h	2008-08-06 13:49:54.000000000 +0900
> +++ linux-2.6.27-rc2-freeze/include/linux/fs.h	2008-08-07 08:59:54.000000000 +0900
> @@ -226,6 +226,8 @@ extern int dir_notify_enable;
>  #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
>  #define FIBMAP	   _IO(0x00,1)	/* bmap access */
>  #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
> +#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
> +#define FITHAW		_IOWR('X', 120, int)	/* Thaw */

FIFREEZE is 119, but a few lines above we have

#define BLKDISCARD _IO(0x12,119)

Should we be using 120 and 121 here?

>  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
>  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
> @@ -574,6 +576,10 @@ struct block_device {
>  	 * care to not mess up bd_private for that case.
>  	 */
>  	unsigned long		bd_private;
> +	/* The counter of freeze processes */
> +	int			bd_freeze_count;
> +	/* Semaphore for freeze */
> +	struct semaphore	bd_freeze_sem;

"freeze" is not an adequate description of what this protects.  I think
it's only the modification and testing of bd_freeze_count, isn't it?

If so, all this could be done more neatly by removing the lock,
switching to atomic_t and using our (rich) atomic_t operations.

otoh, perhaps it protects more than this, in which case the lock
can/should be switched to a `struct mutex'?

Re: [PATCH 1/3] Implement generic freeze feature

[Andreas Dilger]

On Aug 21, 2008  12:58 -0700, Andrew Morton wrote:
> >  #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
> > +#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
> > +#define FITHAW		_IOWR('X', 120, int)	/* Thaw */
> 
> FIFREEZE is 119, but a few lines above we have
> 
> #define BLKDISCARD _IO(0x12,119)
>
> Should we be using 120 and 121 here?

No, because 'X' != 0x12...  The 'X' is used because this ioctl is compatible
with the XFS implementation of this feature.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

Hi,

Andrew Morton wrote:
>> --- linux-2.6.27-rc2.org/include/linux/fs.h  2008-08-06 13:49:54.000000000 +0900
>> +++ linux-2.6.27-rc2-freeze/include/linux/fs.h       2008-08-07 08:59:54.000000000 +0900
>> @@ -226,6 +226,8 @@ extern int dir_notify_enable;
>>  #define BMAP_IOCTL 1                /* obsolete - kept for compatibility */
>>  #define FIBMAP         _IO(0x00,1)  /* bmap access */
>>  #define FIGETBSZ   _IO(0x00,2)      /* get the block size used for bmap */
>> +#define FIFREEZE    _IOWR('X', 119, int)    /* Freeze */
>> +#define FITHAW              _IOWR('X', 120, int)    /* Thaw */
>
>FIFREEZE is 119, but a few lines above we have
>
>#define BLKDISCARD _IO(0x12,119)
>
>Should we be using 120 and 121 here?

As Andreas said, we need to use 'X' to keep compatibility with
XFS's freeze ioctl.

>
>>  #define     FS_IOC_GETFLAGS                 _IOR('f', 1, long)
>>  #define     FS_IOC_SETFLAGS                 _IOW('f', 2, long)
>> @@ -574,6 +576,10 @@ struct block_device {
>>       * care to not mess up bd_private for that case.
>>       */
>>      unsigned long           bd_private;
>> +    /* The counter of freeze processes */
>> +    int                     bd_freeze_count;
>> +    /* Semaphore for freeze */
>> +    struct semaphore        bd_freeze_sem;
>
>"freeze" is not an adequate description of what this protects.  I think
>it's only the modification and testing of bd_freeze_count, isn't it?
>
>If so, all this could be done more neatly by removing the lock,
>switching to atomic_t and using our (rich) atomic_t operations.
>
>otoh, perhaps it protects more than this, in which case the lock
>can/should be switched to a `struct mutex'?

bd_freeze_sem protects the following two sequences.
1. freeze_bdev()
  - Test of bd_freeze_count
  - Increment of bd_freeze_count
  - s_op->write_super_lockfs
  - Set unfreeze timer

2. thaw_bdev()
  - Test of bd_freeze_count
  - Decrement of bd_freeze_count
  - s_op->unlockfs
  - Unset unfreeze timer
Because the journal sync in ext3's write_super_lockfs might
need a long time, we should use the mutex (not atomic_t).
If bd_freeze_sem protects only the modification and
testing of bd_freeze_count, freeze_bdev() and thaw_bdev() will
run simultaneously and unexpected problem will occur. 
(For example, after we run the freeze ioctl with timeout period,
the filesystem is frozen, but the unfreeze timer isn't set.)

Cheers, Takashi

Re: [PATCH 1/3] Implement generic freeze feature

[Christoph Hellwig]

On Mon, Aug 18, 2008 at 09:28:19PM +0900, Takashi Sato wrote:
> +	down(&bdev->bd_freeze_sem);
> +	bdev->bd_freeze_count++;
> +	if (bdev->bd_freeze_count > 1) {
> +		sb = get_super(bdev);
> +		drop_super(sb);
> +		up(&bdev->bd_freeze_sem);
> +		return sb;
> +	}
> +
>  	down(&bdev->bd_mount_sem);

Now you have a reference counter of freezes which actually is pretty
sensible, but also needs some documentation.  What I don't understand
here at all is why you do the get_super/drop_super in the already frozen
case.

Now that the freeze_count has replaced one of the uses of bd_mount_sem
you should also replace the other use in the unmount path by simply
checking for the freez_count and abort if it's set.  To do so you'll
need to hold the bd_mount_sem over the whole unmount operation to
prevent new frezes from coming in.

As others noted it should be a mutex and not a semaphore.

>  /*
> + * ioctl_freeze - Freeze the filesystem.
> + *
> + * @filp:	target file
> + *
> + * Call freeze_bdev() to freeze the filesystem.
> + */
> +static int ioctl_freeze(struct file *filp)

This is not quite kerneldcoc format, which would ne a /** as commnt
start.  But I don't think the comment is actually needed, it's a pretty
obvious file scope function. (Same commnt also applies to ioctl_thaw)

> +	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	/* If filesystem doesn't support freeze feature, return. */
> +	if (sb->s_op->write_super_lockfs == NULL)
> +		return -EOPNOTSUPP;
> +
> +	/* If a regular file or a directory isn't specified, return. */
> +	if (sb->s_bdev == NULL)
> +		return -EINVAL;

I don't understand this commnt.  What you are checking is that the
filesystem has a non-NULL s_bdev, which implies a not blockdevice-backed
filesystem.

Re: [PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

Hi,

Christoph Hellwig wrote:
>On Mon, Aug 18, 2008 at 09:28:19PM +0900, Takashi Sato wrote:
>> +    down(&bdev->bd_freeze_sem);
>> +    bdev->bd_freeze_count++;
>> +    if (bdev->bd_freeze_count > 1) {
>> +            sb = get_super(bdev);
>> +            drop_super(sb);
>> +            up(&bdev->bd_freeze_sem);
>> +            return sb;
>> +    }
>> +
>>      down(&bdev->bd_mount_sem);
>
>Now you have a reference counter of freezes which actually is pretty
>sensible, but also needs some documentation.  What I don't understand
>here at all is why you do the get_super/drop_super in the already frozen
>case.

Even if the filesystem has already been frozen, the superblock
should be returned. Because a caller should recognize the success of
freeze_bdev() and call thaw_bdev() to decrease the reference count. 
But I will remove drop_super() as it should be called in thaw_bdev().

>
>Now that the freeze_count has replaced one of the uses of bd_mount_sem
>you should also replace the other use in the unmount path by simply
>checking for the freez_count and abort if it's set.  To do so you'll
>need to hold the bd_mount_sem over the whole unmount operation to
>prevent new frezes from coming in.

In the original implementation,
unmount is protected by s_umount(semaphore),
not bd_mount_sem.  So, unmount task waits for unfreeze.
I think this original behavior shouldn't be changed, 
so the existing s_umount lock is better.

>
>As others noted it should be a mutex and not a semaphore.

As you said, we should use the mutex.
I will replace it.

>
>>  /*
>> + * ioctl_freeze - Freeze the filesystem.
>> + *
>> + * @filp:   target file
>> + *
>> + * Call freeze_bdev() to freeze the filesystem.
>> + */
>> +static int ioctl_freeze(struct file *filp)
>
>This is not quite kerneldcoc format, which would ne a /** as commnt
>start.  But I don't think the comment is actually needed, it's a pretty
>obvious file scope function. (Same commnt also applies to ioctl_thaw)

I will remove these comments.

>
>> +    struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
>> +
>> +    if (!capable(CAP_SYS_ADMIN))
>> +            return -EPERM;
>> +
>> +    /* If filesystem doesn't support freeze feature, return. */
>> +    if (sb->s_op->write_super_lockfs == NULL)
>> +            return -EOPNOTSUPP;
>> +
>> +    /* If a regular file or a directory isn't specified, return. */
>> +    if (sb->s_bdev == NULL)
>> +            return -EINVAL;
>
>I don't understand this commnt.  What you are checking is that the
>filesystem has a non-NULL s_bdev, which implies a not blockdevice-backed
>filesystem.

I will fix the comment as :
" If a blockdevice-backed filesystem isn't specified, return."

Cheers, Takashi

Re: [PATCH 1/3] Implement generic freeze feature

[Eric Sandeen]

Takashi Sato wrote:

> @@ -141,6 +142,57 @@ static int ioctl_fioasync(unsigned int f
>  }
>  
>  /*
> + * ioctl_freeze - Freeze the filesystem.
> + *
> + * @filp:	target file
> + *
> + * Call freeze_bdev() to freeze the filesystem.
> + */
> +static int ioctl_freeze(struct file *filp)
> +{
> +	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	/* If filesystem doesn't support freeze feature, return. */
> +	if (sb->s_op->write_super_lockfs == NULL)
> +		return -EOPNOTSUPP;
> +
> +	/* If a regular file or a directory isn't specified, return. */
> +	if (sb->s_bdev == NULL)
> +		return -EINVAL;
> +
> +	/* Freeze */
> +	sb = freeze_bdev(sb->s_bdev);
> +	if (IS_ERR(sb))
> +		return PTR_ERR(sb);
> +	return 0;
> +}

Not a problem with your patch exactly, but I was just wondering; you
check here whether the sb returned from freeze_bdev is an ERR_PTR (as
does lock_fs()) - but, freeze_bdev never returns an error, does it?
->write_super_lockfs is a void...

It really seems that at least we should be able to handle IO errors on
the freeze request, and tell the user "No, your filesystem was not
frozen..."?

Maybe I'll whip up a patch to see about propagating freeze errors up
from the filesystems that implement it, unless I'm missing some reason
not to do so...?

Also, should this be checking for a NULL returned from freeze_bdev as
well?  I guess this should never happen if we have a file open on which
we are calling the ioctl ...

-Eric

Re: [PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

Hi,

Eric Sandeen:
>> +static int ioctl_freeze(struct file *filp)
>> +{
>> +     struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
>> +
>> +     if (!capable(CAP_SYS_ADMIN))
>> +         return -EPERM;
>> +
>> +     /* If filesystem doesn't support freeze feature, return. */
>> +     if (sb->s_op->write_super_lockfs == NULL)
>> +         return -EOPNOTSUPP;
>> +
>> +     /* If a regular file or a directory isn't specified, return. */
>> +     if (sb->s_bdev == NULL)
>> +         return -EINVAL;
>> +
>> +     /* Freeze */
>> +     sb = freeze_bdev(sb->s_bdev);
>> +     if (IS_ERR(sb))
>> +         return PTR_ERR(sb);
>> +     return 0;
>> +}
>
> Not a problem with your patch exactly, but I was just wondering; you
> check here whether the sb returned from freeze_bdev is an ERR_PTR (as
> does lock_fs()) - but, freeze_bdev never returns an error, does it?
> ->write_super_lockfs is a void...
>
> It really seems that at least we should be able to handle IO errors on
> the freeze request, and tell the user "No, your filesystem was not
> frozen..."?
>
> Maybe I'll whip up a patch to see about propagating freeze errors up
> from the filesystems that implement it, unless I'm missing some reason
> not to do so...?

Right.
We should handle an IO error which occurs in write_super_lockfs.
I will change the write_super_lockfs's type to "int" so that it can return an error.
And I will consider returning an error of ext3_write_super_lockfs because
journal_flush() in ext3_write_super_lockfs() doesn't handle an IO error.

> Also, should this be checking for a NULL returned from freeze_bdev as
> well?  I guess this should never happen if we have a file open on which
> we are calling the ioctl ...

I think ioctl_freeze doesn't need to check NULL because it never happen
as you said.

Cheers, Takashi

[PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

The ioctls for the generic freeze feature are below.
o Freeze the filesystem
  int ioctl(int fd, int FIFREEZE, arg)
    fd: The file descriptor of the mountpoint
    FIFREEZE: request code for the freeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

o Unfreeze the filesystem
  int ioctl(int fd, int FITHAW, arg)
    fd: The file descriptor of the mountpoint
    FITHAW: request code for unfreeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

Signed-off-by: Takashi Sato <t-sato@yk.jp.nec.com>
Signed-off-by: Masayuki Hamaguchi <m-hamaguchi@ys.jp.nec.com>
---
 fs/block_dev.c              |    2 +
 fs/buffer.c                 |   34 ++++++++++++++++++++++++++++++-
 fs/ioctl.c                  |   47 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/buffer_head.h |    2 -
 include/linux/fs.h          |    7 ++++++
 5 files changed, 90 insertions(+), 2 deletions(-)

diff -uprN -X linux-2.6.27-rc5.org/Documentation/dontdiff linux-2.6.27-rc5.org/fs/block_dev.c linux-2.6.27-rc5-freeze/fs
/block_dev.c
--- linux-2.6.27-rc5.org/fs/block_dev.c	2008-08-29 07:52:02.000000000 +0900
+++ linux-2.6.27-rc5-freeze/fs/block_dev.c	2008-09-05 20:00:29.000000000 +0900
@@ -285,6 +285,8 @@ static void init_once(void *foo)
 	INIT_LIST_HEAD(&bdev->bd_holder_list);
 #endif
 	inode_init_once(&ei->vfs_inode);
+	/* Initialize mutex for freeze. */
+	mutex_init(&bdev->bd_fsfreeze_mutex);
 }
 
 static inline void __bd_forget(struct inode *inode)
diff -uprN -X linux-2.6.27-rc5.org/Documentation/dontdiff linux-2.6.27-rc5.org/fs/buffer.c linux-2.6.27-rc5-freeze/fs/bu
ffer.c
--- linux-2.6.27-rc5.org/fs/buffer.c	2008-08-29 07:52:02.000000000 +0900
+++ linux-2.6.27-rc5-freeze/fs/buffer.c	2008-09-05 20:23:13.000000000 +0900
@@ -196,11 +196,25 @@ int fsync_bdev(struct block_device *bdev
  * happen on bdev until thaw_bdev() is called.
  * If a superblock is found on this device, we take the s_umount semaphore
  * on it to make sure nobody unmounts until the snapshot creation is done.
+ * The reference counter (bd_fsfreeze_count) guarantees that only the last
+ * unfreeze process can unfreeze the frozen filesystem actually when multiple
+ * freeze requests arrive simultaneously. It counts up in freeze_bdev() and
+ * count down in thaw_bdev(). When it becomes 0, thaw_bdev() will unfreeze
+ * actually.
  */
 struct super_block *freeze_bdev(struct block_device *bdev)
 {
 	struct super_block *sb;
 
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	if (bdev->bd_fsfreeze_count > 0) {
+		bdev->bd_fsfreeze_count++;
+		sb = get_super(bdev);
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return sb;
+	}
+	bdev->bd_fsfreeze_count++;
+
 	down(&bdev->bd_mount_sem);
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
@@ -219,6 +233,8 @@ struct super_block *freeze_bdev(struct b
 	}
 
 	sync_blockdev(bdev);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+
 	return sb;	/* thaw_bdev releases s->s_umount and bd_mount_sem */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -230,8 +246,22 @@ EXPORT_SYMBOL(freeze_bdev);
  *
  * Unlocks the filesystem and marks it writeable again after freeze_bdev().
  */
-void thaw_bdev(struct block_device *bdev, struct super_block *sb)
+int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	if (!bdev->bd_fsfreeze_count) {
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return 0;
+	}
+
+	bdev->bd_fsfreeze_count--;
+	if (bdev->bd_fsfreeze_count > 0) {
+		if (sb)
+			drop_super(sb);
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return 0;
+	}
+
 	if (sb) {
 		BUG_ON(sb->s_bdev != bdev);
 
@@ -244,6 +274,8 @@ void thaw_bdev(struct block_device *bdev
 	}
 
 	up(&bdev->bd_mount_sem);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	return 0;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
diff -uprN -X linux-2.6.27-rc5.org/Documentation/dontdiff linux-2.6.27-rc5.org/fs/ioctl.c linux-2.6.27-rc5-freeze/fs/ioc
tl.c
--- linux-2.6.27-rc5.org/fs/ioctl.c	2008-08-29 07:52:02.000000000 +0900
+++ linux-2.6.27-rc5-freeze/fs/ioctl.c	2008-09-05 20:22:46.000000000 +0900
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/buffer_head.h>
 
 #include <asm/ioctls.h>
 
@@ -140,6 +141,43 @@ static int ioctl_fioasync(unsigned int f
 	return error;
 }
 
+static int ioctl_freeze(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support freeze feature, return. */
+	if (sb->s_op->write_super_lockfs == NULL)
+		return -EOPNOTSUPP;
+
+	/* If a blockdevice-backed filesystem isn't specified, return. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	/* Freeze */
+	sb = freeze_bdev(sb->s_bdev);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+	return 0;
+}
+
+static int ioctl_thaw(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	/* Thaw */
+	return thaw_bdev(sb->s_bdev, sb);
+}
+
 /*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
@@ -181,6 +219,15 @@ int do_vfs_ioctl(struct file *filp, unsi
 		} else
 			error = -ENOTTY;
 		break;
+
+	case FIFREEZE:
+		error = ioctl_freeze(filp);
+		break;
+
+	case FITHAW:
+		error = ioctl_thaw(filp);
+		break;
+
 	default:
 		if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
 			error = file_ioctl(filp, cmd, arg);
diff -uprN -X linux-2.6.27-rc5.org/Documentation/dontdiff linux-2.6.27-rc5.org/include/linux/buffer_head.h linux-2.6.27-
rc5-freeze/include/linux/buffer_head.h
--- linux-2.6.27-rc5.org/include/linux/buffer_head.h	2008-08-29 07:52:02.000000000 +0900
+++ linux-2.6.27-rc5-freeze/include/linux/buffer_head.h	2008-09-05 20:16:12.000000000 +0900
@@ -170,7 +170,7 @@ void __wait_on_buffer(struct buffer_head
 wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 int fsync_bdev(struct block_device *);
 struct super_block *freeze_bdev(struct block_device *);
-void thaw_bdev(struct block_device *, struct super_block *);
+int thaw_bdev(struct block_device *, struct super_block *);
 int fsync_super(struct super_block *);
 int fsync_no_super(struct block_device *);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
diff -uprN -X linux-2.6.27-rc5.org/Documentation/dontdiff linux-2.6.27-rc5.org/include/linux/fs.h linux-2.6.27-rc5-freez
e/include/linux/fs.h
--- linux-2.6.27-rc5.org/include/linux/fs.h	2008-08-29 07:52:02.000000000 +0900
+++ linux-2.6.27-rc5-freeze/include/linux/fs.h	2008-09-05 20:18:21.000000000 +0900
@@ -226,6 +226,8 @@ extern int dir_notify_enable;
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
+#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
+#define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
@@ -574,6 +576,11 @@ struct block_device {
 	 * care to not mess up bd_private for that case.
 	 */
 	unsigned long		bd_private;
+
+	/* The counter of freeze processes */
+	int			bd_fsfreeze_count;
+	/* Mutex for freeze */
+	struct mutex		bd_fsfreeze_mutex;
 };
 
 /*

Re: [PATCH 1/3] Implement generic freeze feature

[Christoph Hellwig]

On Mon, Sep 08, 2008 at 08:52:45PM +0900, Takashi Sato wrote:
> diff -uprN -X linux-2.6.27-rc5.org/Documentation/dontdiff linux-2.6.27-rc5.org/fs/block_dev.c linux-2.6.27-rc5-freeze/fs
> /block_dev.c
> --- linux-2.6.27-rc5.org/fs/block_dev.c	2008-08-29 07:52:02.000000000 +0900
> +++ linux-2.6.27-rc5-freeze/fs/block_dev.c	2008-09-05 20:00:29.000000000 +0900
> @@ -285,6 +285,8 @@ static void init_once(void *foo)
>  	INIT_LIST_HEAD(&bdev->bd_holder_list);
>  #endif
>  	inode_init_once(&ei->vfs_inode);
> +	/* Initialize mutex for freeze. */
> +	mutex_init(&bdev->bd_fsfreeze_mutex);

Why not just freeze_mutex?


>  struct super_block *freeze_bdev(struct block_device *bdev)
>  {
>  	struct super_block *sb;
>  
> +	mutex_lock(&bdev->bd_fsfreeze_mutex);
> +	if (bdev->bd_fsfreeze_count > 0) {
> +		bdev->bd_fsfreeze_count++;
> +		sb = get_super(bdev);
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return sb;
> +	}
> +	bdev->bd_fsfreeze_count++;
> +
>  	down(&bdev->bd_mount_sem);

Note that we still have duplication with the bd_mount_sem.  I think
you should look into getting rid of it and instead do a check of
the freeze_count under proper freeze_mutex protection.

> +int thaw_bdev(struct block_device *bdev, struct super_block *sb)
>  {
> +	mutex_lock(&bdev->bd_fsfreeze_mutex);
> +	if (!bdev->bd_fsfreeze_count) {
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return 0;
> +	}
> +
> +	bdev->bd_fsfreeze_count--;
> +	if (bdev->bd_fsfreeze_count > 0) {
> +		if (sb)
> +			drop_super(sb);
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return 0;
> +	}
> +
>  	if (sb) {
>  		BUG_ON(sb->s_bdev != bdev);
>  
> @@ -244,6 +274,8 @@ void thaw_bdev(struct block_device *bdev
>  	}
>  
>  	up(&bdev->bd_mount_sem);
> +	mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +	return 0;

Why do you add a return value here if we always return 0 anyway?

Re: [PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

Hi,

Christoph Hellwig wrote:
>> --- linux-2.6.27-rc5.org/fs/block_dev.c 2008-08-29 07:52:02.000000000 +0900
>> +++ linux-2.6.27-rc5-freeze/fs/block_dev.c 2008-09-05 20:00:29.000000000 +0900
>> @@ -285,6 +285,8 @@ static void init_once(void *foo)
>>      INIT_LIST_HEAD(&bdev->bd_holder_list);
>>  #endif
>>      inode_init_once(&ei->vfs_inode);
>> +     /* Initialize mutex for freeze. */
>> +     mutex_init(&bdev->bd_fsfreeze_mutex);
>
> Why not just freeze_mutex?

The Linux kernel has already had the name of "freezer" in the part of
power-management.  So I named the above mutex "fsfreeze"
(filesystem freeze) to distinguish it from the existent "freezer".
Andrew pointed it out.

>>  struct super_block *freeze_bdev(struct block_device *bdev)
>>  {
>>       struct super_block *sb;
>>
>> +    mutex_lock(&bdev->bd_fsfreeze_mutex);
>> +    if (bdev->bd_fsfreeze_count > 0) {
>> +        bdev->bd_fsfreeze_count++;
>> +        sb = get_super(bdev);
>> +        mutex_unlock(&bdev->bd_fsfreeze_mutex);
>> +        return sb;
>> +    }
>> +    bdev->bd_fsfreeze_count++;
>> +
>>      down(&bdev->bd_mount_sem);
>
> Note that we still have duplication with the bd_mount_sem.  I think
> you should look into getting rid of it and instead do a check of
> the freeze_count under proper freeze_mutex protection.

In the original implementation,
while the filesystem is frozen, subsequent mounts wait for unfreeze
with the semaphore (bd_mount_sem).
But if we replace the semphore with the check of the freeze_count,
subsequent mounts will abort.
I think the original behavior shouldn't be changed, so the existing bd_mount_sem
is better.

>> @@ -244,6 +274,8 @@ void thaw_bdev(struct block_device *bdev
>>  }
>>
>>        up(&bdev->bd_mount_sem);
>> +     mutex_unlock(&bdev->bd_fsfreeze_mutex);
>> +     return 0;
>
> Why do you add a return value here if we always return 0 anyway?

I forgot to remove the unneeded return value in above old patch.
But I need to implement a return value in the new patch
because thaw_bdev() needs to return an IO error which occurs
in unlockfs().
Eric pointed it out.

Cheers, Takashi

[PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

When multiple freeze requests arrive simultaneously, only the last
unfreeze process should unfreeze the frozen filesystem actually
(as Dave Chinner, Eric Sandeen and Alasdair G Kergon commented).
So I've added the reference counter to the freeze feature.
It counts up in freeze_bdev() and counts down in thaw_bdev().
When it becomes "0", thaw_bdev() will unfreeze actually.

The ioctls for the generic freeze feature are below.
o Freeze the filesystem
  int ioctl(int fd, int FIFREEZE, arg)
    fd: The file descriptor of the mountpoint
    FIFREEZE: request code for the freeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

o Unfreeze the filesystem
  int ioctl(int fd, int FITHAW, arg)
    fd: The file descriptor of the mountpoint
    FITHAW: request code for unfreeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

Signed-off-by: Takashi Sato <t-sato@yk.jp.nec.com>
Signed-off-by: Masayuki Hamaguchi <m-hamaguchi@ys.jp.nec.com>
---
 fs/block_dev.c              |    2 +
 fs/buffer.c                 |   27 ++++++++++++++++++-
 fs/ioctl.c                  |   61 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/buffer_head.h |    2 -
 include/linux/fs.h          |    6 ++++
 5 files changed, 96 insertions(+), 2 deletions(-)

diff -uprN -X linux-2.6.26.org/Documentation/dontdiff linux-2.6.26.org/fs/block_dev.c linux-2.6.26-freeze/fs/block_dev.c
--- linux-2.6.26.org/fs/block_dev.c	2008-07-14 06:51:29.000000000 +0900
+++ linux-2.6.26-freeze/fs/block_dev.c	2008-07-18 15:38:03.000000000 +0900
@@ -285,6 +285,8 @@ static void init_once(struct kmem_cache 
 	INIT_LIST_HEAD(&bdev->bd_holder_list);
 #endif
 	inode_init_once(&ei->vfs_inode);
+	/* Initialize semaphore for freeze. */
+	sema_init(&bdev->bd_freeze_sem, 1);
 }
 
 static inline void __bd_forget(struct inode *inode)
diff -uprN -X linux-2.6.26.org/Documentation/dontdiff linux-2.6.26.org/fs/buffer.c linux-2.6.26-freeze/fs/buffer.c
--- linux-2.6.26.org/fs/buffer.c	2008-07-14 06:51:29.000000000 +0900
+++ linux-2.6.26-freeze/fs/buffer.c	2008-07-18 15:45:17.000000000 +0900
@@ -201,6 +201,15 @@ struct super_block *freeze_bdev(struct b
 {
 	struct super_block *sb;
 
+	down(&bdev->bd_freeze_sem);
+	bdev->bd_freeze_count++;
+	if (bdev->bd_freeze_count > 1) {
+		sb = get_super(bdev);
+		drop_super(sb);
+		up(&bdev->bd_freeze_sem);
+		return sb;
+	}
+
 	down(&bdev->bd_mount_sem);
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
@@ -219,6 +228,8 @@ struct super_block *freeze_bdev(struct b
 	}
 
 	sync_blockdev(bdev);
+	up(&bdev->bd_freeze_sem);
+
 	return sb;	/* thaw_bdev releases s->s_umount and bd_mount_sem */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -230,8 +241,20 @@ EXPORT_SYMBOL(freeze_bdev);
  *
  * Unlocks the filesystem and marks it writeable again after freeze_bdev().
  */
-void thaw_bdev(struct block_device *bdev, struct super_block *sb)
+int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
+
+	down(&bdev->bd_freeze_sem);
+	if (!bdev->bd_freeze_count) {
+		up(&bdev->bd_freeze_sem);
+		return 0;
+	}
+	bdev->bd_freeze_count--;
+	if (bdev->bd_freeze_count > 0) {
+		up(&bdev->bd_freeze_sem);
+		return 0;
+	}
+
 	if (sb) {
 		BUG_ON(sb->s_bdev != bdev);
 
@@ -244,6 +267,8 @@ void thaw_bdev(struct block_device *bdev
 	}
 
 	up(&bdev->bd_mount_sem);
+	up(&bdev->bd_freeze_sem);
+	return 0;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
diff -uprN -X linux-2.6.26.org/Documentation/dontdiff linux-2.6.26.org/fs/ioctl.c linux-2.6.26-freeze/fs/ioctl.c
--- linux-2.6.26.org/fs/ioctl.c	2008-07-14 06:51:29.000000000 +0900
+++ linux-2.6.26-freeze/fs/ioctl.c	2008-07-18 21:24:42.000000000 +0900
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/buffer_head.h>
 
 #include <asm/ioctls.h>
 
@@ -141,6 +142,57 @@ static int ioctl_fioasync(unsigned int f
 }
 
 /*
+ * ioctl_freeze - Freeze the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call freeze_bdev() to freeze the filesystem.
+ */
+static int ioctl_freeze(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support freeze feature, return. */
+	if (sb->s_op->write_super_lockfs == NULL)
+		return -EOPNOTSUPP;
+
+	/* If a regular file or a directory isn't specified, return. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	/* Freeze */
+	sb = freeze_bdev(sb->s_bdev);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+	return 0;
+}
+
+/*
+ * ioctl_thaw - Thaw the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call thaw_bdev() to thaw the filesystem.
+ */
+static int ioctl_thaw(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If a regular file or a directory isn't specified, return EINVAL. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	/* Thaw */
+	return thaw_bdev(sb->s_bdev, sb);
+}
+
+/*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
  *
@@ -181,6 +233,15 @@ int do_vfs_ioctl(struct file *filp, unsi
 		} else
 			error = -ENOTTY;
 		break;
+
+	case FIFREEZE:
+		error = ioctl_freeze(filp);
+		break;
+
+	case FITHAW:
+		error = ioctl_thaw(filp);
+		break;
+
 	default:
 		if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
 			error = file_ioctl(filp, cmd, arg);
diff -uprN -X linux-2.6.26.org/Documentation/dontdiff linux-2.6.26.org/include/linux/buffer_head.h linux-2.6.26-freeze/i
nclude/linux/buffer_head.h
--- linux-2.6.26.org/include/linux/buffer_head.h	2008-07-14 06:51:29.000000000 +0900
+++ linux-2.6.26-freeze/include/linux/buffer_head.h	2008-07-17 11:32:37.000000000 +0900
@@ -171,7 +171,7 @@ void __wait_on_buffer(struct buffer_head
 wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 int fsync_bdev(struct block_device *);
 struct super_block *freeze_bdev(struct block_device *);
-void thaw_bdev(struct block_device *, struct super_block *);
+int thaw_bdev(struct block_device *, struct super_block *);
 int fsync_super(struct super_block *);
 int fsync_no_super(struct block_device *);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
diff -uprN -X linux-2.6.26.org/Documentation/dontdiff linux-2.6.26.org/include/linux/fs.h linux-2.6.26-freeze/include/li
nux/fs.h
--- linux-2.6.26.org/include/linux/fs.h	2008-07-14 06:51:29.000000000 +0900
+++ linux-2.6.26-freeze/include/linux/fs.h	2008-07-18 15:34:46.000000000 +0900
@@ -224,6 +224,8 @@ extern int dir_notify_enable;
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
+#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
+#define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
@@ -548,6 +550,10 @@ struct block_device {
 	 * care to not mess up bd_private for that case.
 	 */
 	unsigned long		bd_private;
+	/* The counter of freeze processes */
+	int			bd_freeze_count;
+	/* Semaphore for freeze */
+	struct semaphore	bd_freeze_sem;
 };
 
 /*

[PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

The ioctls for the generic freeze feature are below.
o Freeze the filesystem
  int ioctl(int fd, int FIFREEZE, arg)
    fd: The file descriptor of the mountpoint
    FIFREEZE: request code for the freeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

o Unfreeze the filesystem
  int ioctl(int fd, int FITHAW, arg)
    fd: The file descriptor of the mountpoint
    FITHAW: request code for unfreeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

Signed-off-by: Takashi Sato <t-sato@yk.jp.nec.com>
Signed-off-by: Masayuki Hamaguchi <m-hamaguchi@ys.jp.nec.com>
---
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/fs/buffer.c linux-2.6.26-rc7-freeze/fs/bu
ffer.c
--- linux-2.6.26-rc7.org/fs/buffer.c	2008-06-25 12:08:10.000000000 +0900
+++ linux-2.6.26-rc7-freeze/fs/buffer.c	2008-06-27 09:18:19.000000000 +0900
@@ -201,6 +201,21 @@ struct super_block *freeze_bdev(struct b
 {
 	struct super_block *sb;
 
+	if (test_and_set_bit(BD_FREEZE_OP, &bdev->bd_state))
+		return ERR_PTR(-EBUSY);
+
+	sb = get_super(bdev);
+
+	/* If super_block has been already frozen, return. */
+	if (sb && sb->s_frozen != SB_UNFROZEN) {
+		drop_super(sb);
+		clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+		return sb;
+	}
+
+	if (sb)
+		drop_super(sb);
+
 	down(&bdev->bd_mount_sem);
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
@@ -219,6 +234,8 @@ struct super_block *freeze_bdev(struct b
 	}
 
 	sync_blockdev(bdev);
+	clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+
 	return sb;	/* thaw_bdev releases s->s_umount and bd_mount_sem */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -230,8 +247,17 @@ EXPORT_SYMBOL(freeze_bdev);
  *
  * Unlocks the filesystem and marks it writeable again after freeze_bdev().
  */
-void thaw_bdev(struct block_device *bdev, struct super_block *sb)
+int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
+
+	if (test_and_set_bit(BD_FREEZE_OP, &bdev->bd_state))
+		return -EBUSY;
+
+	if (sb && sb->s_frozen == SB_UNFROZEN) {
+		clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+		return 0;
+	}
+
 	if (sb) {
 		BUG_ON(sb->s_bdev != bdev);
 
@@ -244,6 +270,8 @@ void thaw_bdev(struct block_device *bdev
 	}
 
 	up(&bdev->bd_mount_sem);
+	clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+	return 0;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/fs/ioctl.c linux-2.6.26-rc7-freeze/fs/ioc
tl.c
--- linux-2.6.26-rc7.org/fs/ioctl.c	2008-06-25 12:08:14.000000000 +0900
+++ linux-2.6.26-rc7-freeze/fs/ioctl.c	2008-06-25 12:00:41.000000000 +0900
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/buffer_head.h>
 
 #include <asm/ioctls.h>
 
@@ -141,6 +142,49 @@ static int ioctl_fioasync(unsigned int f
 }
 
 /*
+ * ioctl_freeze - Freeze the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call freeze_bdev() to freeze the filesystem.
+ */
+static int ioctl_freeze(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support freeze feature, return. */
+	if (sb->s_op->write_super_lockfs == NULL)
+		return -EOPNOTSUPP;
+
+	/* Freeze */
+	sb = freeze_bdev(sb->s_bdev);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+	return 0;
+}
+
+/*
+ * ioctl_thaw - Thaw the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call thaw_bdev() to thaw the filesystem.
+ */
+static int ioctl_thaw(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* Thaw */
+	return thaw_bdev(sb->s_bdev, sb);
+}
+
+/*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
  *
@@ -181,6 +225,15 @@ int do_vfs_ioctl(struct file *filp, unsi
 		} else
 			error = -ENOTTY;
 		break;
+
+	case FIFREEZE:
+		error = ioctl_freeze(filp);
+		break;
+
+	case FITHAW:
+		error = ioctl_thaw(filp);
+		break;
+
 	default:
 		if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
 			error = file_ioctl(filp, cmd, arg);
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/include/linux/buffer_head.h linux-2.6.26-
rc7-freeze/include/linux/buffer_head.h
--- linux-2.6.26-rc7.org/include/linux/buffer_head.h	2008-06-25 12:08:19.000000000 +0900
+++ linux-2.6.26-rc7-freeze/include/linux/buffer_head.h	2008-06-25 12:00:45.000000000 +0900
@@ -171,7 +171,7 @@ void __wait_on_buffer(struct buffer_head
 wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 int fsync_bdev(struct block_device *);
 struct super_block *freeze_bdev(struct block_device *);
-void thaw_bdev(struct block_device *, struct super_block *);
+int thaw_bdev(struct block_device *, struct super_block *);
 int fsync_super(struct super_block *);
 int fsync_no_super(struct block_device *);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/include/linux/fs.h linux-2.6.26-rc7-freez
e/include/linux/fs.h
--- linux-2.6.26-rc7.org/include/linux/fs.h	2008-06-25 12:08:19.000000000 +0900
+++ linux-2.6.26-rc7-freeze/include/linux/fs.h	2008-06-30 16:40:49.000000000 +0900
@@ -223,6 +223,8 @@ extern int dir_notify_enable;
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
+#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
+#define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
@@ -494,6 +496,13 @@ int pagecache_write_end(struct file *, s
 				loff_t pos, unsigned len, unsigned copied,
 				struct page *page, void *fsdata);
 
+/*
+ * Bits in block_device.bd_state.
+ */
+enum bd_state {
+	BD_FREEZE_OP	/* In freeze operation */
+};
+
 struct backing_dev_info;
 struct address_space {
 	struct inode		*host;		/* owner: inode, block_device */
@@ -547,6 +556,9 @@ struct block_device {
 	 * care to not mess up bd_private for that case.
 	 */
 	unsigned long		bd_private;
+
+	/* State of the block device. (Used by freeze feature) */
+	unsigned long		bd_state;
 };
 
 /*

Re: [PATCH 1/3] Implement generic freeze feature

[Christoph Hellwig]

>  {
>  	struct super_block *sb;
>  
> +	if (test_and_set_bit(BD_FREEZE_OP, &bdev->bd_state))
> +		return ERR_PTR(-EBUSY);
> +
> +	sb = get_super(bdev);
> +
> +	/* If super_block has been already frozen, return. */
> +	if (sb && sb->s_frozen != SB_UNFROZEN) {
> +		drop_super(sb);
> +		clear_bit(BD_FREEZE_OP, &bdev->bd_state);
> +		return sb;
> +	}
> +
> +	if (sb)
> +		drop_super(sb);
> +
>  	down(&bdev->bd_mount_sem);
>  	sb = get_super(bdev);
>  	if (sb && !(sb->s_flags & MS_RDONLY)) {
> @@ -219,6 +234,8 @@ struct super_block *freeze_bdev(struct b
>  	}
>  
>  	sync_blockdev(bdev);
> +	clear_bit(BD_FREEZE_OP, &bdev->bd_state);
> +

Please only clear BD_FREEZE_OP in thaw_bdev, that way you can also get
rid of the frozen check above, and the double-get_super.  Also
bd_mount_sem could be removed that way by checking for BD_FREEZE_OP
in the unmount path.

>  /*
> + * ioctl_freeze - Freeze the filesystem.
> + *
> + * @filp:	target file
> + *
> + * Call freeze_bdev() to freeze the filesystem.
> + */

This is not a kerneldoc comment.  But I think it can be simply removed
anyway, as it's a quite trivial function with static scope.

> +/*
> + * ioctl_thaw - Thaw the filesystem.
> + *
> + * @filp:	target file
> + *
> + * Call thaw_bdev() to thaw the filesystem.
> + */

Same here.

[PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

I have modified to set the suitable error number (EOPNOTSUPP)
in case the filesystem doesn't support the freeze feature.
This was pointed out by Andreas Dilger.

The ioctls for the generic freeze feature are below.
o Freeze the filesystem
  int ioctl(int fd, int FIFREEZE, arg)
    fd: The file descriptor of the mountpoint
    FIFREEZE: request code for the freeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

o Unfreeze the filesystem
  int ioctl(int fd, int FITHAW, arg)
    fd: The file descriptor of the mountpoint
    FITHAW: request code for unfreeze
    arg: Ignored
    Return value: 0 if the operation succeeds. Otherwise, -1

Signed-off-by: Takashi Sato <t-sato@yk.jp.nec.com>
Signed-off-by: Masayuki Hamaguchi <m-hamaguchi@ys.jp.nec.com>
---
 fs/buffer.c                 |   30 ++++++++++++++++++++++++
 fs/ioctl.c                  |   53 ++++++++++++++++++++++++++++++++++++++++++++
 fs/super.c                  |   32 +++++++++++++++++++++++++-
 include/linux/buffer_head.h |    2 -
 include/linux/fs.h          |   14 +++++++++++
 5 files changed, 128 insertions(+), 3 deletions(-)

diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/fs/buffer.c linux-2.6.26-rc7-freeze/fs/bu
ffer.c
--- linux-2.6.26-rc7.org/fs/buffer.c	2008-06-21 08:19:44.000000000 +0900
+++ linux-2.6.26-rc7-freeze/fs/buffer.c	2008-06-23 11:54:48.000000000 +0900
@@ -201,6 +201,21 @@ struct super_block *freeze_bdev(struct b
 {
 	struct super_block *sb;
 
+	if (test_and_set_bit(BD_FREEZE_OP, &bdev->bd_state))
+		return ERR_PTR(-EBUSY);
+
+	sb = get_super_without_lock(bdev);
+
+	/* If super_block has been already frozen, return. */
+	if (sb && sb->s_frozen != SB_UNFROZEN) {
+		put_super(sb);
+		clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+		return sb;
+	}
+
+	if (sb)
+		put_super(sb);
+
 	down(&bdev->bd_mount_sem);
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
@@ -219,6 +234,8 @@ struct super_block *freeze_bdev(struct b
 	}
 
 	sync_blockdev(bdev);
+	clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+
 	return sb;	/* thaw_bdev releases s->s_umount and bd_mount_sem */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -230,8 +247,17 @@ EXPORT_SYMBOL(freeze_bdev);
  *
  * Unlocks the filesystem and marks it writeable again after freeze_bdev().
  */
-void thaw_bdev(struct block_device *bdev, struct super_block *sb)
+int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
+
+	if (test_and_set_bit(BD_FREEZE_OP, &bdev->bd_state))
+		return -EBUSY;
+
+	if (sb && sb->s_frozen == SB_UNFROZEN) {
+		clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+		return 0;
+	}
+
 	if (sb) {
 		BUG_ON(sb->s_bdev != bdev);
 
@@ -244,6 +270,8 @@ void thaw_bdev(struct block_device *bdev
 	}
 
 	up(&bdev->bd_mount_sem);
+	clear_bit(BD_FREEZE_OP, &bdev->bd_state);
+	return 0;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/fs/ioctl.c linux-2.6.26-rc7-freeze/fs/ioc
tl.c
--- linux-2.6.26-rc7.org/fs/ioctl.c	2008-06-21 08:19:44.000000000 +0900
+++ linux-2.6.26-rc7-freeze/fs/ioctl.c	2008-06-23 11:54:48.000000000 +0900
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/buffer_head.h>
 
 #include <asm/ioctls.h>
 
@@ -141,6 +142,49 @@ static int ioctl_fioasync(unsigned int f
 }
 
 /*
+ * ioctl_freeze - Freeze the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call freeze_bdev() to freeze the filesystem.
+ */
+static int ioctl_freeze(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support freeze feature, return. */
+	if (sb->s_op->write_super_lockfs == NULL)
+		return -EOPNOTSUPP;
+
+	/* Freeze */
+	sb = freeze_bdev(sb->s_bdev);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+	return 0;
+}
+
+/*
+ * ioctl_thaw - Thaw the filesystem.
+ *
+ * @filp:	target file
+ *
+ * Call thaw_bdev() to thaw the filesystem.
+ */
+static int ioctl_thaw(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* Thaw */
+	return thaw_bdev(sb->s_bdev, sb);
+}
+
+/*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
  *
@@ -181,6 +225,15 @@ int do_vfs_ioctl(struct file *filp, unsi
 		} else
 			error = -ENOTTY;
 		break;
+
+	case FIFREEZE:
+		error = ioctl_freeze(filp);
+		break;
+
+	case FITHAW:
+		error = ioctl_thaw(filp);
+		break;
+
 	default:
 		if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
 			error = file_ioctl(filp, cmd, arg);
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/fs/super.c linux-2.6.26-rc7-freeze/fs/sup
er.c
--- linux-2.6.26-rc7.org/fs/super.c	2008-06-21 08:19:44.000000000 +0900
+++ linux-2.6.26-rc7-freeze/fs/super.c	2008-06-23 11:54:48.000000000 +0900
@@ -156,7 +156,7 @@ int __put_super_and_need_restart(struct 
  *	Drops a temporary reference, frees superblock if there's no
  *	references left.
  */
-static void put_super(struct super_block *sb)
+void put_super(struct super_block *sb)
 {
 	spin_lock(&sb_lock);
 	__put_super(sb);
@@ -509,6 +509,36 @@ rescan:
 
 EXPORT_SYMBOL(get_super);
  
+/*
+ * get_super_without_lock - Get super_block from block_device without lock.
+ * @bdev:	block device struct
+ *
+ * Scan the superblock list and finds the superblock of the file system
+ * mounted on the block device given. This doesn't lock anyone.
+ * %NULL is returned if no match is found.
+ */
+struct super_block *get_super_without_lock(struct block_device *bdev)
+{
+	struct super_block *sb;
+
+	if (!bdev)
+		return NULL;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &super_blocks, s_list) {
+		if (sb->s_bdev == bdev) {
+			if (sb->s_root) {
+				sb->s_count++;
+				spin_unlock(&sb_lock);
+				return sb;
+			}
+		}
+	}
+	spin_unlock(&sb_lock);
+	return NULL;
+}
+EXPORT_SYMBOL(get_super_without_lock);
+
 struct super_block * user_get_super(dev_t dev)
 {
 	struct super_block *sb;
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/include/linux/buffer_head.h linux-2.6.26-
rc7-freeze/include/linux/buffer_head.h
--- linux-2.6.26-rc7.org/include/linux/buffer_head.h	2008-06-21 08:19:44.000000000 +0900
+++ linux-2.6.26-rc7-freeze/include/linux/buffer_head.h	2008-06-23 11:54:48.000000000 +0900
@@ -171,7 +171,7 @@ void __wait_on_buffer(struct buffer_head
 wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 int fsync_bdev(struct block_device *);
 struct super_block *freeze_bdev(struct block_device *);
-void thaw_bdev(struct block_device *, struct super_block *);
+int thaw_bdev(struct block_device *, struct super_block *);
 int fsync_super(struct super_block *);
 int fsync_no_super(struct block_device *);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7.org/include/linux/fs.h linux-2.6.26-rc7-freez
e/include/linux/fs.h
--- linux-2.6.26-rc7.org/include/linux/fs.h	2008-06-21 08:19:44.000000000 +0900
+++ linux-2.6.26-rc7-freeze/include/linux/fs.h	2008-06-23 11:54:48.000000000 +0900
@@ -223,6 +223,8 @@ extern int dir_notify_enable;
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
+#define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
+#define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
@@ -494,6 +496,13 @@ int pagecache_write_end(struct file *, s
 				loff_t pos, unsigned len, unsigned copied,
 				struct page *page, void *fsdata);
 
+/*
+ * Bits in block_device.bd_state.
+ */
+enum bd_state {
+	BD_FREEZE_OP	/* In freeze operation */
+};
+
 struct backing_dev_info;
 struct address_space {
 	struct inode		*host;		/* owner: inode, block_device */
@@ -547,6 +556,9 @@ struct block_device {
 	 * care to not mess up bd_private for that case.
 	 */
 	unsigned long		bd_private;
+
+	/* State of the block device. (Used by freeze feature) */
+	unsigned long		bd_state;
 };
 
 /*
@@ -1964,7 +1976,9 @@ extern int do_vfs_ioctl(struct file *fil
 extern void get_filesystem(struct file_system_type *fs);
 extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
+extern void put_super(struct super_block *sb);
 extern struct super_block *get_super(struct block_device *);
+extern struct super_block *get_super_without_lock(struct block_device *);
 extern struct super_block *user_get_super(dev_t);
 extern void drop_super(struct super_block *sb);
 

Re: [PATCH 1/3] Implement generic freeze feature

[Andrew Morton]

On Tue, 24 Jun 2008 15:59:50 +0900
Takashi Sato <t-sato@yk.jp.nec.com> wrote:

> I have modified to set the suitable error number (EOPNOTSUPP)
> in case the filesystem doesn't support the freeze feature.
> This was pointed out by Andreas Dilger.
> 
> The ioctls for the generic freeze feature are below.
> o Freeze the filesystem
>   int ioctl(int fd, int FIFREEZE, arg)
>     fd: The file descriptor of the mountpoint
>     FIFREEZE: request code for the freeze
>     arg: Ignored
>     Return value: 0 if the operation succeeds. Otherwise, -1
> 
> o Unfreeze the filesystem
>   int ioctl(int fd, int FITHAW, arg)
>     fd: The file descriptor of the mountpoint
>     FITHAW: request code for unfreeze
>     arg: Ignored
>     Return value: 0 if the operation succeeds. Otherwise, -1
>
> ...
>
> +/*
> + * get_super_without_lock - Get super_block from block_device without lock.
> + * @bdev:	block device struct
> + *
> + * Scan the superblock list and finds the superblock of the file system
> + * mounted on the block device given. This doesn't lock anyone.
> + * %NULL is returned if no match is found.
> + */

This is not a terribly good comment.

Which lock are we not taking?  I _assume_ that it's referring to
s_umount?  If so, the text should describe that.

It should also go to some lengths explaining why this dangerous-looking
and rather nasty-looking function exists.

Look at it this way: there is no way in which the reviewer of this
patch (ie: me) can work out why this function exists.  Hence there will
be no way in which future readers of this code will be able to work out
why this function exists either.  This is bad.  These things should be
described in code comments and in the changelog (whichever is most
appropriate).

> +struct super_block *get_super_without_lock(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +
> +	if (!bdev)
> +		return NULL;
> +
> +	spin_lock(&sb_lock);
> +	list_for_each_entry(sb, &super_blocks, s_list) {
> +		if (sb->s_bdev == bdev) {
> +			if (sb->s_root) {
> +				sb->s_count++;
> +				spin_unlock(&sb_lock);
> +				return sb;
> +			}
> +		}
> +	}
> +	spin_unlock(&sb_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(get_super_without_lock);

Re: [PATCH 1/3] Implement generic freeze feature

[Takashi Sato]

Hi,

>> +/*
>> + * get_super_without_lock - Get super_block from block_device without lock.
>> + * @bdev: block device struct
>> + *
>> + * Scan the superblock list and finds the superblock of the file system
>> + * mounted on the block device given. This doesn't lock anyone.
>> + * %NULL is returned if no match is found.
>> + */
>
> This is not a terribly good comment.
>
> Which lock are we not taking?  I _assume_ that it's referring to
> s_umount?  If so, the text should describe that.
>
> It should also go to some lengths explaining why this dangerous-looking
> and rather nasty-looking function exists.
>
> Look at it this way: there is no way in which the reviewer of this
> patch (ie: me) can work out why this function exists.  Hence there will
> be no way in which future readers of this code will be able to work out
> why this function exists either.  This is bad.  These things should be
> described in code comments and in the changelog (whichever is most
> appropriate).

Thank you for your comment.  I will write comments appropriately.

I was wrong. I thought we didn't need to lock s_umount because
this ioctl required to open a regular file or a directory and we cannot
unmount a target filesystem.
So I created get_super_without_lock() used in freeze_bdev().

But, I have found that the ioctl (DM_DEV_SUSPEND_CMD in
drivers/md/dm-ioctl.c) requires to open  a logical volume
(not a file or a directory) and calls freeze_bdev(), so we can unmount
a filesystem.
So I will replace get_super_without_lock with get_super to get s_umount.

Cheers, Takashi