On Tue, 24 Jun 2008 16:00:56 +0900
Takashi Sato <t-sato@yk.jp.nec.com> wrote:

> The timeout feature is added to freeze ioctl. And new ioctl
> to reset the timeout period is added.
> o Freeze the filesystem
> int ioctl(int fd, int FIFREEZE, long *timeval)
> fd: The file descriptor of the mountpoint
> FIFREEZE: request code for the freeze
> timeval: the timeout period in seconds
> If it's 0 or 1, the timeout isn't set.
> This special case of "1" is implemented to keep
> the compatibility with XFS applications.
> Return value: 0 if the operation succeeds. Otherwise, -1
>
> o Reset the timeout period
> int ioctl(int fd, int FIFREEZE_RESET_TIMEOUT, long *timeval)
> fd:file descriptor of mountpoint
> FIFREEZE_RESET_TIMEOUT: request code for reset of timeout period
> timeval: new timeout period in seconds
> Return value: 0 if the operation succeeds. Otherwise, -1
> Error number: If the filesystem has already been unfrozen,
> errno is set to EINVAL.
>

Please avoid the use of the term `timeval' here. That term is closely
associated with `struct timeval', and these are not being used here. A
better identifier would be timeout_secs - it tells the reader what it
does, and it tells the reader what units it is in. And when it comes
to "time", it is very useful to tell people which units are being used,
because there are many different ones.

>
> ...
>
> --- linux-2.6.26-rc7-xfs/fs/buffer.c 2008-06-23 11:55:15.000000000 +0900
> +++ linux-2.6.26-rc7-timeout/fs/buffer.c 2008-06-23 11:56:47.000000000 +0900
> @@ -190,14 +190,17 @@ int fsync_bdev(struct block_device *bdev
>
> /**
> * freeze_bdev -- lock a filesystem and force it into a consistent state
> - * @bdev: blockdevice to lock
> + * @bdev: blockdevice to lock
> + * @timeout_msec: timeout period
> *
> * This takes the block device bd_mount_sem to make sure no new mounts
> * 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.
> + * If timeout_msec is bigger than 0, this registers the delayed work for
> + * timeout of the freeze feature.
> */
> -struct super_block *freeze_bdev(struct block_device *bdev)
> +struct super_block *freeze_bdev(struct block_device *bdev, long timeout_msec)

timeout_msec is good.

> {
> struct super_block *sb;
>
> @@ -234,6 +237,10 @@ struct super_block *freeze_bdev(struct b
> }
>
> sync_blockdev(bdev);
> + /* Setup unfreeze timer. */
> + if (timeout_msec > 0)
> + add_freeze_timeout(bdev, timeout_msec);
> +
> clear_bit(BD_FREEZE_OP, &bdev->bd_state);
>
> return sb; /* thaw_bdev releases s->s_umount and bd_mount_sem */
> @@ -258,6 +265,9 @@ int thaw_bdev(struct block_device *bdev,
> return 0;
> }
>
> + /* Delete unfreeze timer. */
> + del_freeze_timeout(bdev);
> +
> if (sb) {
> BUG_ON(sb->s_bdev != bdev);
>
> diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/fs/ioctl.c linux-2.6.26-rc7-timeout/fs/io
> ctl.c
> --- linux-2.6.26-rc7-xfs/fs/ioctl.c 2008-06-23 11:55:15.000000000 +0900
> +++ linux-2.6.26-rc7-timeout/fs/ioctl.c 2008-06-23 11:56:47.000000000 +0900
> @@ -145,12 +145,16 @@ static int ioctl_fioasync(unsigned int f
> * ioctl_freeze - Freeze the filesystem.
> *
> * @filp: target file
> + * @argp: timeout value(sec)
> *
> * Call freeze_bdev() to freeze the filesystem.
> */
> -static int ioctl_freeze(struct file *filp)
> +static int ioctl_freeze(struct file *filp, unsigned long arg)
> {
> + long timeout_sec;
> + long timeout_msec;
> struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> + int error;
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> @@ -159,8 +163,27 @@ static int ioctl_freeze(struct file *fil
> if (sb->s_op->write_super_lockfs == NULL)
> return -EOPNOTSUPP;
>
> + /* arg(sec) to tick value. */
> + error = get_user(timeout_sec, (long __user *) arg);

uh-oh, compat problems.

A 32-bit application running under a 32-bit kernel will need to provide
a 32-bit quantity.

A 32-bit application running under a 64-bit kernel will need to provide
a 64-bit quantity.

I suggest that you go through the entire patch and redo this part of
the kernel ABI in terms of __u32, and avoid any mention of "long" in
the kernel<->userspace interface.

> + if (error != 0)
> + return error;
> + /*
> + * If 1 is specified as the timeout period,
> + * it will be changed into 0 to keep the compatibility
> + * of XFS application(xfs_freeze).
> + */
> + if (timeout_sec < 0)
> + return -EINVAL;
> + else if (timeout_sec < 2)
> + timeout_sec = 0;

The `else' is unneeded.

It would be clearer to code this all as:

if (timeout_sec < 0)
return -EINVAL;

if (timeout_sec == 1) {
/*
* If 1 is specified as the timeout period it is changed into 0
* to retain compatibility with XFS's xfs_freeze.
*/
timeout_sec = 0;
}


> + timeout_msec = timeout_sec * 1000;
> + /* overflow case */
> + if (timeout_msec < 0)
> + return -EINVAL;
> +
> /* Freeze */
> - sb = freeze_bdev(sb->s_bdev);
> + sb = freeze_bdev(sb->s_bdev, timeout_msec);
> if (IS_ERR(sb))
> return PTR_ERR(sb);
> return 0;
> @@ -185,6 +208,51 @@ static int ioctl_thaw(struct file *filp)
> }
>
> /*
> + * ioctl_freeze_reset_timeout - Reset timeout for freeze.
> + *
> + * @filp: target file
> + * @argp: timeout value(sec)
> + *
> + * Rest timeout for freeze.

typo

> + */
> +static int
> +ioctl_freeze_reset_timeout(struct file *filp, unsigned long arg)
> +{
> + long timeout_sec;
> + long timeout_msec;
> + struct super_block *sb
> + = filp->f_path.dentry->d_inode->i_sb;

Unneeded linebreak there.

> + int error;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + /* arg(sec) to tick value */
> + error = get_user(timeout_sec, (long __user *) arg);

compat issues.

> + if (error)
> + return error;
> + timeout_msec = timeout_sec * 1000;
> + if (timeout_msec < 0)
> + return -EINVAL;

um, OK, but timeout_msec could have overflowed during the
multiplication and could be complete garbage. I guess it doesn't
matter much, but a cleaner approach might be:

if (timeout_sec > LONG_MAX/1000)
return -EINVAL;

or something like that.

But otoh, why do the multiplication at all? If we're able to handle
the timeout down to the millisecond level, why not offer that
resolution to userspace? Offer the increased time granularity to the
application?

> + if (sb) {

Can this happen?

> + if (test_and_set_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state))
> + return -EBUSY;
> + if (sb->s_frozen == SB_UNFROZEN) {
> + clear_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state);
> + return -EINVAL;
> + }
> + /* setup unfreeze timer */
> + if (timeout_msec > 0)

What does this test do? afacit it's special-casing the timeout_secs==0
case. Was this feature documented? What does it do?

> + add_freeze_timeout(sb->s_bdev,
> + timeout_msec);
> + clear_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state);
> + }
> +
> + return 0;
> +}
> +
>
> ...
>
> +void add_freeze_timeout(struct block_device *bdev, long timeout_msec)
> +{
> + s64 timeout_jiffies = msecs_to_jiffies(timeout_msec);
> +
> + /* Set delayed work queue */
> + cancel_delayed_work(&bdev->bd_freeze_timeout);

Should this have used cancel_delayed_work_sync()?

> + schedule_delayed_work(&bdev->bd_freeze_timeout, timeout_jiffies);
> +}
> +
> +/*
> + * del_freeze_timeout - Delete timeout for freeze.
> + *
> + * @bdev: block device struct
> + *
> + * Delete the delayed work for freeze timeout from the delayed work queue.
> + */
> +void del_freeze_timeout(struct block_device *bdev)
> +{
> + if (delayed_work_pending(&bdev->bd_freeze_timeout))

Is this test needed?

> + cancel_delayed_work(&bdev->bd_freeze_timeout);

cancel_delayed_work_sync()?

> +}
> diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/fs/xfs/xfs_fsops.c linux-2.6.26-rc7-timeo
> ut/fs/xfs/xfs_fsops.c
> --- linux-2.6.26-rc7-xfs/fs/xfs/xfs_fsops.c 2008-06-23 11:55:15.000000000 +0900
> +++ linux-2.6.26-rc7-timeout/fs/xfs/xfs_fsops.c 2008-06-23 11:56:47.000000000 +0900
> @@ -619,7 +619,7 @@ xfs_fs_goingdown(
> {
> switch (inflags) {
> case XFS_FSOP_GOING_FLAGS_DEFAULT: {
> - struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
> + struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, 0);

Using NULL here is clearer and will, I expect, avoid a sparse warning.

>
> ...
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi,


o Reset the timeout period
int ioctl(int fd, int FIFREEZE_RESET_TIMEOUT, long *timeval)
fd:file descriptor of mountpoint
FIFREEZE_RESET_TIMEOUT: request code for reset of timeout period
timeval: new timeout period in seconds
Return value: 0 if the operation succeeds. Otherwise, -1
Error number: If the filesystem has already been unfrozen,
errno is set to EINVAL.


Please avoid the use of the term `timeval' here. That term is closely
associated with `struct timeval', and these are not being used here. A
better identifier would be timeout_secs - it tells the reader what it
does, and it tells the reader what units it is in. And when it comes
to "time", it is very useful to tell people which units are being used,
because there are many different ones.


OK.
I will use "timeout_secs" in the next version to match the source code.


-static int ioctl_freeze(struct file *filp)
+static int ioctl_freeze(struct file *filp, unsigned long arg)
{
+ long timeout_sec;
+ long timeout_msec;
struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+ int error;

if (!capable(CAP_SYS_ADMIN))
return -EPERM;
@@ -159,8 +163,27 @@ static int ioctl_freeze(struct file *fil
if (sb->s_op->write_super_lockfs == NULL)
return -EOPNOTSUPP;

+ /* arg(sec) to tick value. */
+ error = get_user(timeout_sec, (long __user *) arg);>

uh-oh, compat problems.

A 32-bit application running under a 32-bit kernel will need to provide
a 32-bit quantity.

A 32-bit application running under a 64-bit kernel will need to provide
a 64-bit quantity.

I suggest that you go through the entire patch and redo this part of
the kernel ABI in terms of __u32, and avoid any mention of "long" in
the kernel<->userspace interface.


I agree.
I will replace long with a 32bit integer type.


+ /* arg(sec) to tick value. */
+ error = get_user(timeout_sec, (long __user *) arg);
+ if (timeout_sec < 0)
+ return -EINVAL;
+ else if (timeout_sec < 2)
+ timeout_sec = 0;> The `else' is unneeded.


It would be clearer to code this all as:

if (timeout_sec < 0)
return -EINVAL;

if (timeout_sec == 1) {
/*
* If 1 is specified as the timeout period it is changed into 0
* to retain compatibility with XFS's xfs_freeze.
*/
timeout_sec = 0;
}


OK.


+ if (error)
+ return error;
+ timeout_msec = timeout_sec * 1000;
+ if (timeout_msec < 0)
+ return -EINVAL;


um, OK, but timeout_msec could have overflowed during the
multiplication and could be complete garbage. I guess it doesn't
matter much, but a cleaner approach might be:

if (timeout_sec > LONG_MAX/1000)
return -EINVAL;

or something like that.


OK.


But otoh, why do the multiplication at all? If we're able to handle
the timeout down to the millisecond level, why not offer that
resolution to userspace? Offer the increased time granularity to the
application?


I think there is no case the user specifies it in millisecond,
so the second granularity is more comfortable.


+ if (sb) {


Can this happen?


I have found that sb == NULL never happen
but sb->s_bdev == NULL can happen when we call this ioctl
for a device file (not a directory or a regular file).
So I will add the following check.
if (sb->s_bdev == NULL)
return -EINVAL;


+ if (test_and_set_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state))
+ return -EBUSY;
+ if (sb->s_frozen == SB_UNFROZEN) {
+ clear_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state);
+ return -EINVAL;
+ }
+ /* setup unfreeze timer */
+ if (timeout_msec > 0)


What does this test do? afacit it's special-casing the timeout_secs==0
case. Was this feature documented? What does it do?


There is no special case of timeout_secs==0.
I will remove this check.


+void add_freeze_timeout(struct block_device *bdev, long timeout_msec)
+{
+ s64 timeout_jiffies = msecs_to_jiffies(timeout_msec);
+
+ /* Set delayed work queue */
+ cancel_delayed_work(&bdev->bd_freeze_timeout);


Should this have used cancel_delayed_work_sync()?


Exactly.
I will replace cancel_delayed_work with cancel_delayed_work_sync.


+void del_freeze_timeout(struct block_device *bdev)
+{
+ if (delayed_work_pending(&bdev->bd_freeze_timeout))


Is this test needed?


It's not necessary because cancel_delayed_work_sync checks it itself.
I will remove it.


--- linux-2.6.26-rc7-xfs/fs/xfs/xfs_fsops.c 2008-06-23 11:55:15.000000000 +0900
+++ linux-2.6.26-rc7-timeout/fs/xfs/xfs_fsops.c 2008-06-23 11:56:47.000000000 +0900
@@ -619,7 +619,7 @@ xfs_fs_goingdown(
{
switch (inflags) {
case XFS_FSOP_GOING_FLAGS_DEFAULT: {
- struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
+ struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, 0);


Using NULL here is clearer and will, I expect, avoid a sparse warning.


I checked it but I couldn't find a sparse warning in xfs_fsops.c.
Can you tell me how to use NULL?

Cheers, Takashi

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Fri, 27 Jun 2008 20:33:58 +0900
"Takashi Sato" <t-sato@yk.jp.nec.com> wrote:

> >> case XFS_FSOP_GOING_FLAGS_DEFAULT: {
> >> - struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
> >> + struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, 0);
> >
> > Using NULL here is clearer and will, I expect, avoid a sparse warning.
>
> I checked it but I couldn't find a sparse warning in xfs_fsops.c.
> Can you tell me how to use NULL?

struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, NULL);



It's much better to use NULL here rather than literal zero because the
reader of this code can then say "ah-hah, we're passing in a pointer".
Whereas plain old "0" could be a pointer or a scalar.

We should always use NULL to represent a null pointer in the kernel.
The one acceptable exception is when testing for nullness:

if (ptr1)
if (!ptr2)

Often people will use

if (ptr1 != NULL)
if (ptr2 == NULL)

in this case as well. (I prefer the shorter version personally, but
either is OK).


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi,


>> case XFS_FSOP_GOING_FLAGS_DEFAULT: {
>> - struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
>> + struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, 0);
>
> Using NULL here is clearer and will, I expect, avoid a sparse warning.

I checked it but I couldn't find a sparse warning in xfs_fsops.c.
Can you tell me how to use NULL?


struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, NULL);



It's much better to use NULL here rather than literal zero because the
reader of this code can then say "ah-hah, we're passing in a pointer".
Whereas plain old "0" could be a pointer or a scalar.


The second argument's type of freeze_bdev() is "long", not pointer as below.
struct super_block *freeze_bdev(struct block_device *, long timeout_msec);

So "0" is reasonable, isn't it?

We should always use NULL to represent a null pointer in the kernel.
The one acceptable exception is when testing for nullness:


if (ptr1)
if (!ptr2)

Often people will use

if (ptr1 != NULL)
if (ptr2 == NULL)

in this case as well. (I prefer the shorter version personally, but
either is OK).


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Mon, 30 Jun 2008 08:13:07 +0900 "Takashi Sato" <t-sato@yk.jp.nec.com> wrote:

> > It's much better to use NULL here rather than literal zero because the
> > reader of this code can then say "ah-hah, we're passing in a pointer".
> > Whereas plain old "0" could be a pointer or a scalar.
>
> The second argument's type of freeze_bdev() is "long", not pointer as below.
> struct super_block *freeze_bdev(struct block_device *, long timeout_msec);

oh, ok, I goofed, sorry.

> So "0" is reasonable, isn't it?

yup.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

The timeout feature is added to freeze ioctl. And new ioctl
to reset the timeout period is added.
o Freeze the filesystem
int ioctl(int fd, int FIFREEZE, long *timeout_sec)
fd: The file descriptor of the mountpoint
FIFREEZE: request code for the freeze
timeout_sec: the timeout period in seconds
If it's 0 or 1, the timeout isn't set.
This special case of "1" is implemented to keep
the compatibility with XFS applications.
Return value: 0 if the operation succeeds. Otherwise, -1

o Reset the timeout period
int ioctl(int fd, int FIFREEZE_RESET_TIMEOUT, long *timeout_sec)
fd:file descriptor of mountpoint
FIFREEZE_RESET_TIMEOUT: request code for reset of timeout period
timeout_sec: new timeout period in seconds
Return value: 0 if the operation succeeds. Otherwise, -1
Error number: If the filesystem has already been unfrozen,
errno is set to EINVAL.

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-xfs/drivers/md/dm.c linux-2.6.26-rc7-timeout/
drivers/md/dm.c
--- linux-2.6.26-rc7-xfs/drivers/md/dm.c 2008-06-25 12:06:59.000000000 +0900
+++ linux-2.6.26-rc7-timeout/drivers/md/dm.c 2008-06-25 16:28:40.000000000 +0900
@@ -1407,7 +1407,7 @@ static int lock_fs(struct mapped_device

WARN_ON(md->frozen_sb);

- md->frozen_sb = freeze_bdev(md->suspended_bdev);
+ md->frozen_sb = freeze_bdev(md->suspended_bdev, 0);
if (IS_ERR(md->frozen_sb)) {
r = PTR_ERR(md->frozen_sb);
md->frozen_sb = NULL;
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/fs/block_dev.c linux-2.6.26-rc7-timeout/f
s/block_dev.c
--- linux-2.6.26-rc7-xfs/fs/block_dev.c 2008-06-25 12:07:19.000000000 +0900
+++ linux-2.6.26-rc7-timeout/fs/block_dev.c 2008-06-25 16:30:49.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);
+ /* Setup freeze timeout function. */
+ INIT_DELAYED_WORK(&bdev->bd_freeze_timeout, freeze_timeout);
}

static inline void __bd_forget(struct inode *inode)
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/fs/buffer.c linux-2.6.26-rc7-timeout/fs/b
uffer.c
--- linux-2.6.26-rc7-xfs/fs/buffer.c 2008-06-30 12:59:53.000000000 +0900
+++ linux-2.6.26-rc7-timeout/fs/buffer.c 2008-06-27 10:59:55.000000000 +0900
@@ -190,14 +190,18 @@ int fsync_bdev(struct block_device *bdev

/**
* freeze_bdev -- lock a filesystem and force it into a consistent state
- * @bdev: blockdevice to lock
+ * @bdev: blockdevice to lock
+ * @timeout_msec: timeout period
*
* This takes the block device bd_mount_sem to make sure no new mounts
* 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.
+ * If timeout_msec is bigger than 0, this registers the delayed work for
+ * timeout of the freeze feature.
*/
-struct super_block *freeze_bdev(struct block_device *bdev)
+struct super_block *freeze_bdev(struct block_device *bdev,
+ unsigned int timeout_msec)
{
struct super_block *sb;

@@ -234,6 +238,10 @@ struct super_block *freeze_bdev(struct b
}

sync_blockdev(bdev);
+ /* Setup unfreeze timer. */
+ if (timeout_msec > 0)
+ add_freeze_timeout(bdev, timeout_msec);
+
clear_bit(BD_FREEZE_OP, &bdev->bd_state);

return sb; /* thaw_bdev releases s->s_umount and bd_mount_sem */
@@ -258,6 +266,9 @@ int thaw_bdev(struct block_device *bdev,
return 0;
}

+ /* Delete unfreeze timer. */
+ del_freeze_timeout(bdev);
+
if (sb) {
BUG_ON(sb->s_bdev != bdev);

diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/fs/ioctl.c linux-2.6.26-rc7-timeout/fs/io
ctl.c
--- linux-2.6.26-rc7-xfs/fs/ioctl.c 2008-06-25 12:07:20.000000000 +0900
+++ linux-2.6.26-rc7-timeout/fs/ioctl.c 2008-06-27 08:49:58.000000000 +0900
@@ -145,22 +145,47 @@ static int ioctl_fioasync(unsigned int f
* ioctl_freeze - Freeze the filesystem.
*
* @filp: target file
+ * @argp: timeout value(sec)
*
* Call freeze_bdev() to freeze the filesystem.
*/
-static int ioctl_freeze(struct file *filp)
+static int ioctl_freeze(struct file *filp, int __user *argp)
{
+ int timeout_sec;
+ unsigned int timeout_msec;
struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+ int error;

if (!capable(CAP_SYS_ADMIN))
return -EPERM;

- /* If filesystem doesn't support freeze feature, return. */
+ /* If filesystem doesn't support freeze feature, return EOPNOTSUPP. */
if (sb->s_op->write_super_lockfs == NULL)
return -EOPNOTSUPP;

+ /* If a regular file or a directory isn't specified, return EINVAL. */
+ if (sb->s_bdev == NULL)
+ return -EINVAL;
+
+ /* arg(sec) to tick value. */
+ error = get_user(timeout_sec, argp);
+ if (error != 0)
+ return error;
+
+ if (timeout_sec < 0 || timeout_sec > UINT_MAX/1000)
+ return -EINVAL;
+
+ /*
+ * If 1 is specified as the timeout period it is changed into 0
+ * to retain compatibility with XFS's xfs_freeze.
+ */
+ if (timeout_sec == 1)
+ timeout_sec = 0;
+
+ timeout_msec = timeout_sec * 1000;
+
/* Freeze */
- sb = freeze_bdev(sb->s_bdev);
+ sb = freeze_bdev(sb->s_bdev, timeout_msec);
if (IS_ERR(sb))
return PTR_ERR(sb);
return 0;
@@ -180,11 +205,61 @@ static int ioctl_thaw(struct file *filp)
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);
}

/*
+ * ioctl_freeze_reset_timeout - Reset timeout for freeze.
+ *
+ * @filp: target file
+ * @argp: timeout value(sec)
+ *
+ * Reset timeout for freeze.
+ */
+static int
+ioctl_freeze_reset_timeout(struct file *filp, int __user *argp)
+{
+ int timeout_sec;
+ unsigned int timeout_msec;
+ struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+ int error;
+
+ 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;
+
+ /* arg(sec) to tick value */
+ error = get_user(timeout_sec, argp);
+ if (error)
+ return error;
+
+ if (timeout_sec <= 0 || timeout_sec > UINT_MAX/1000)
+ return -EINVAL;
+
+ timeout_msec = timeout_sec * 1000;
+
+ if (test_and_set_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state))
+ return -EBUSY;
+ if (sb->s_frozen == SB_UNFROZEN) {
+ clear_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state);
+ return -EINVAL;
+ }
+ /* setup unfreeze timer */
+ add_freeze_timeout(sb->s_bdev, timeout_msec);
+ clear_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state);
+
+ return 0;
+}
+
+/*
* When you add any new common ioctls to the switches above and below
* please update compat_sys_ioctl() too.
*
@@ -227,13 +302,17 @@ int do_vfs_ioctl(struct file *filp, unsi
break;

case FIFREEZE:
- error = ioctl_freeze(filp);
+ error = ioctl_freeze(filp, argp);
break;

case FITHAW:
error = ioctl_thaw(filp);
break;

+ case FIFREEZE_RESET_TIMEOUT:
+ error = ioctl_freeze_reset_timeout(filp, argp);
+ 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-xfs/fs/super.c linux-2.6.26-rc7-timeout/fs/su
per.c
--- linux-2.6.26-rc7-xfs/fs/super.c 2008-06-30 16:41:48.000000000 +0900
+++ linux-2.6.26-rc7-timeout/fs/super.c 2008-06-30 17:30:38.000000000 +0900
@@ -980,3 +980,60 @@ struct vfsmount *kern_mount_data(struct
}

EXPORT_SYMBOL_GPL(kern_mount_data);
+
+/*
+ * freeze_timeout - Thaw the filesystem.
+ *
+ * @work: work queue (delayed_work.work)
+ *
+ * Called by the delayed work when elapsing the timeout period.
+ * Thaw the filesystem.
+ */
+void freeze_timeout(struct work_struct *work)
+{
+ struct block_device *bd = container_of(work,
+ struct block_device, bd_freeze_timeout.work);
+ struct super_block *sb = get_super(bd);
+
+ thaw_bdev(bd, sb);
+
+ if (sb)
+ drop_super(sb);
+}
+EXPORT_SYMBOL_GPL(freeze_timeout);
+
+/*
+ * add_freeze_timeout - Add timeout for freeze.
+ *
+ * @bdev: block device struct
+ * @timeout_msec: timeout period
+ *
+ * Add the delayed work for freeze timeout to the delayed work queue.
+ */
+void add_freeze_timeout(struct block_device *bdev, unsigned int timeout_msec)
+{
+ s64 timeout_jiffies = msecs_to_jiffies(timeout_msec);
+
+ /* Set delayed work queue */
+ cancel_delayed_work_sync(&bdev->bd_freeze_timeout);
+ schedule_delayed_work(&bdev->bd_freeze_timeout, timeout_jiffies);
+}
+
+/*
+ * del_freeze_timeout - Delete timeout for freeze.
+ *
+ * @bdev: block device struct
+ *
+ * Delete the delayed work for freeze timeout from the delayed work queue.
+ */
+void del_freeze_timeout(struct block_device *bdev)
+{
+ /*
+ * It's possible that the delayed work task (freeze_timeout()) calls
+ * del_freeze_timeout(). If the delayed work task calls
+ * cancel_delayed_work_sync((), the deadlock will occur.
+ * So we need this check (delayed_work_pending()).
+ */
+ if (delayed_work_pending(&bdev->bd_freeze_timeout))
+ cancel_delayed_work_sync(&bdev->bd_freeze_timeout);
+}
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/fs/xfs/xfs_fsops.c linux-2.6.26-rc7-timeo
ut/fs/xfs/xfs_fsops.c
--- linux-2.6.26-rc7-xfs/fs/xfs/xfs_fsops.c 2008-06-25 12:07:19.000000000 +0900
+++ linux-2.6.26-rc7-timeout/fs/xfs/xfs_fsops.c 2008-06-25 16:30:48.000000000 +0900
@@ -619,7 +619,7 @@ xfs_fs_goingdown(
{
switch (inflags) {
case XFS_FSOP_GOING_FLAGS_DEFAULT: {
- struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
+ struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, 0);

if (sb && !IS_ERR(sb)) {
xfs_force_shutdown(mp, SHUTDOWN_FORCE_UMOUNT);
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/include/linux/buffer_head.h linux-2.6.26-
rc7-timeout/include/linux/buffer_head.h
--- linux-2.6.26-rc7-xfs/include/linux/buffer_head.h 2008-06-25 12:07:24.000000000 +0900
+++ linux-2.6.26-rc7-timeout/include/linux/buffer_head.h 2008-06-27 11:11:38.000000000 +0900
@@ -170,7 +170,8 @@ int sync_blockdev(struct block_device *b
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 *);
+struct super_block *freeze_bdev(struct block_device *,
+ unsigned int timeout_msec);
int thaw_bdev(struct block_device *, struct super_block *);
int fsync_super(struct super_block *);
int fsync_no_super(struct block_device *);
diff -uprN -X linux-2.6.26-rc7.org/Documentation/dontdiff linux-2.6.26-rc7-xfs/include/linux/fs.h linux-2.6.26-rc7-timeo
ut/include/linux/fs.h
--- linux-2.6.26-rc7-xfs/include/linux/fs.h 2008-06-30 16:42:11.000000000 +0900
+++ linux-2.6.26-rc7-timeout/include/linux/fs.h 2008-06-30 16:43:13.000000000 +0900
@@ -8,6 +8,7 @@

#include <linux/limits.h>
#include <linux/ioctl.h>
+#include <linux/workqueue.h>

/*
* It's silly to have NR_OPEN bigger than NR_FILE, but you can change
@@ -225,6 +226,7 @@ extern int dir_notify_enable;
#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 FIFREEZE_RESET_TIMEOUT _IO(0x00, 3) /* Reset freeze timeout */

#define FS_IOC_GETFLAGS _IOR('f', 1, long)
#define FS_IOC_SETFLAGS _IOW('f', 2, long)
@@ -559,6 +561,8 @@ struct block_device {

/* State of the block device. (Used by freeze feature) */
unsigned long bd_state;
+ /* Delayed work for freeze */
+ struct delayed_work bd_freeze_timeout;
};

/*
@@ -2147,5 +2151,10 @@ int proc_nr_files(struct ctl_table *tabl

int get_filesystem_list(char * buf);

+extern void add_freeze_timeout(struct block_device *bdev,
+ unsigned int timeout_msec);
+extern void del_freeze_timeout(struct block_device *bdev);
+extern void freeze_timeout(struct work_struct *work);
+
#endif /* __KERNEL__ */
#endif /* _LINUX_FS_H */

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

I still disagree with this whole patch. There is not reason to let
the freeze request timeout - an auto-unfreezing will only confuse the
hell out of the caller. The only reason where the current XFS freeze
call can hang and this would be theoretically useful is when the
filesystem is already frozen by someone else, but this should be fixed
by refusing to do the second freeze, as suggested in my comment to patch
1.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Tue, Jul 01, 2008 at 04:10:26AM -0400, Christoph Hellwig wrote:
> I still disagree with this whole patch.

Same here - if you want a timeout, what stops you from implementing it in a
userspace process? If your concern is that the process might die without
thawing the filesystem, take a look at the userspace LVM/multipath code for
ideas - lock into memory, disable OOM killer, run from ramdisk etc.
In practice, those techniques seem to be good enough.

> call can hang and this would be theoretically useful is when the
> filesystem is already frozen by someone else, but this should be fixed
> by refusing to do the second freeze, as suggested in my comment to patch
> 1.

Similarly if a device-mapper device is involved, how should the following
sequence behave - A, B or C?

1. dmsetup suspend (freezes)
2. FIFREEZE
3. FITHAW
4. dmsetup resume (thaws)

A:
1 succeeds, freezes
2 succeeds, remains frozen
3 succeeds, remains frozen
4 succeeds, thaws

B:
1 succeeds, freezes
2 fails, remains frozen
3 shouldn't be called because 2 failed but if it is: succeeds, thaws
4 succeeds (already thawed, but still does the device-mapper parts)

C:
1 succeeds, freezes
2 fails, remains frozen
3 fails (because device-mapper owns the freeze/thaw), remains frozen
4 succeeds, thaws

Alasdair
--
agk@redhat.com

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi Christoph and Alasdair,


On Tue, Jul 01, 2008 at 04:10:26AM -0400, Christoph Hellwig wrote:

I still disagree with this whole patch.


Same here - if you want a timeout, what stops you from implementing it in a
userspace process? If your concern is that the process might die without
thawing the filesystem, take a look at the userspace LVM/multipath code for
ideas - lock into memory, disable OOM killer, run from ramdisk etc.
In practice, those techniques seem to be good enough.


If the freezer accesses the frozen filesystem and causes a deadlock,
the above ideas can't solve it. The timeout is useful to solve such a deadlock.
If you don't need the timeout, you can disable it by specifying "0" as the
timeout period.


Similarly if a device-mapper device is involved, how should the following
sequence behave - A, B or C?

1. dmsetup suspend (freezes)
2. FIFREEZE
3. FITHAW
4. dmsetup resume (thaws)

[...]

C:
1 succeeds, freezes
2 fails, remains frozen
3 fails (because device-mapper owns the freeze/thaw), remains frozen
4 succeeds, thaws


I think C is appropriate and the following change makes it possible.
How do you think?

1. Add the new bit flag(BD_FREEZE_DM) in block_device.bd_state.
It means that the volume is frozen by the device-mapper.

2. Operate and check this bit flag as followings.

- Bit operations in the device-mapper's freeze/thaw
FREEZE:
dm_suspend(): set BD_FREEZE_DM
freeze_bdev():set BD_FREEZE_OP

THAW:
thaw_bdev(): clear BD_FREEZE_OP
dm_resume(): clear BD_FREEZE_DM

- Checks in FIFREEZE/FITHAW
FREEZE:
ioctl_freeze(): Not need to check BD_FREEZE_DM
freeze_bdev():set BD_FREEZE_OP

THAW:
ioctl_thaw(): If BD_FREEZE_DM is set, fail, otherwise, call thaw_bdev()
thaw_bdev(): clear BD_FREEZE_OP

Cheers, Takashi

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Thu, Jul 03, 2008 at 09:11:05PM +0900, Takashi Sato wrote:
> If the freezer accesses the frozen filesystem and causes a deadlock,
> the above ideas can't solve it

But you could also say that if the 'freezer' process accesses the frozen
filesystem and deadlocks then that's just a bug and that userspace code
should be fixed and there's no need to introduce the complexity of a
timeout parameter.

> >Similarly if a device-mapper device is involved, how should the following
> >sequence behave - A, B or C?
> >
> >1. dmsetup suspend (freezes)
> >2. FIFREEZE
> >3. FITHAW
> >4. dmsetup resume (thaws)
> [...]
> >C:
> > 1 succeeds, freezes
> > 2 fails, remains frozen
> > 3 fails (because device-mapper owns the freeze/thaw), remains frozen
> > 4 succeeds, thaws
>
> I think C is appropriate and the following change makes it possible.
> How do you think?

The point I'm trying to make here is:
Under what real-world circumstances might multiple concurrent freezing
attempts occur, and which of A, B or C (or other variations) would be
the most appropriate way of handling such situations?

A common example is people running xfs_freeze followed by an lvm command
which also attempts to freeze the filesystem.

I can see a case for B or C, but personally I prefer A:

> > 1 succeeds, freezes
> > 2 succeeds, remains frozen
> > 3 succeeds, remains frozen
> > 4 succeeds, thaws

Alasdair
--
agk@redhat.com

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel