[PATCH] fs: add the FIGETFROZEN ioctl call

[PATCH] fs: add the FIGETFROZEN ioctl call

[Florian Margaine]

This lets userland get the filesystem freezing status, aka whether the
filesystem is frozen or not. This is so that an application can know if
it should freeze the filesystem or if it isn't necessary when taking a
snapshot.
---
 fs/compat_ioctl.c       |  1 +
 fs/ioctl.c              | 13 +++++++++++++
 include/uapi/linux/fs.h |  1 +
 3 files changed, 15 insertions(+)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index bd01b92..d2173ab 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -918,6 +918,7 @@ COMPATIBLE_IOCTL(FIGETBSZ)
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
 COMPATIBLE_IOCTL(FITRIM)
+COMPATIBLE_IOCTL(FIGETFROZEN)
 COMPATIBLE_IOCTL(KDGETKEYCODE)
 COMPATIBLE_IOCTL(KDSETKEYCODE)
 COMPATIBLE_IOCTL(KDGKBTYPE)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 116a333..249ed20 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -568,6 +568,16 @@ static int ioctl_fsthaw(struct file *filp)
  return thaw_super(sb);
 }

+static int ioctl_fsgetfrozen(struct file *filp)
+{
+ struct super_block *sb = file_inode(filp)->i_sb;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ return sb->s_writers.frozen;
+}
+
 static long ioctl_file_dedupe_range(struct file *file, void __user *arg)
 {
  struct file_dedupe_range __user *argp = arg;
@@ -652,6 +662,9 @@ int do_vfs_ioctl(struct file *filp, unsigned int
fd, unsigned int cmd,
  error = ioctl_fsthaw(filp);
  break;

+ case FIGETFROZEN:
+ return put_user(ioctl_fsgetfrozen(filp), argp);
+
  case FS_IOC_FIEMAP:
  return ioctl_fiemap(filp, arg);

diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 149bec8..d48f19c 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -230,6 +230,7 @@ struct fsxattr {
 #define FIFREEZE _IOWR('X', 119, int) /* Freeze */
 #define FITHAW _IOWR('X', 120, int) /* Thaw */
 #define FITRIM _IOWR('X', 121, struct fstrim_range) /* Trim */
+#define FIGETFROZEN _IOWR('X', 122, int) /* Frozen status */
 #define FICLONE _IOW(0x94, 9, int)
 #define FICLONERANGE _IOW(0x94, 13, struct file_clone_range)
 #define FIDEDUPERANGE _IOWR(0x94, 54, struct file_dedupe_range)
-- 
2.8.0

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Florian Margaine]

Hi,

For some reason, my email client replaced tabs with spaces in my
previous email.

Here is the patch using another client.

Regards,
Florian


>From 80febe62cf5c21e0a369b64e38c82f068a416a61 Mon Sep 17 00:00:00 2001
From: Florian MARGAINE <florian@margaine.com>
Date: Sat, 19 Mar 2016 23:25:28 +0100
Subject: [PATCH] fs: add the FIGETFROZEN ioctl call

This lets userland get the filesystem freezing status, aka whether the
filesystem is frozen or not. This is so that an application can know if
it should freeze the filesystem or if it isn't necessary when taking a
snapshot.
---
 fs/compat_ioctl.c       |  1 +
 fs/ioctl.c              | 13 +++++++++++++
 include/uapi/linux/fs.h |  1 +
 3 files changed, 15 insertions(+)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index bd01b92..d2173ab 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -918,6 +918,7 @@ COMPATIBLE_IOCTL(FIGETBSZ)
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
 COMPATIBLE_IOCTL(FITRIM)
+COMPATIBLE_IOCTL(FIGETFROZEN)
 COMPATIBLE_IOCTL(KDGETKEYCODE)
 COMPATIBLE_IOCTL(KDSETKEYCODE)
 COMPATIBLE_IOCTL(KDGKBTYPE)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 116a333..249ed20 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -568,6 +568,16 @@ static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb);
 }
 
+static int ioctl_fsgetfrozen(struct file *filp)
+{
+	struct super_block *sb = file_inode(filp)->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	return sb->s_writers.frozen;
+}
+
 static long ioctl_file_dedupe_range(struct file *file, void __user
*arg)
 {
 	struct file_dedupe_range __user *argp = arg;
@@ -652,6 +662,9 @@ int do_vfs_ioctl(struct file *filp, unsigned int
fd, unsigned int cmd,
 		error = ioctl_fsthaw(filp);
 		break;
 
+	case FIGETFROZEN:
+		return put_user(ioctl_fsgetfrozen(filp), argp);
+
 	case FS_IOC_FIEMAP:
 		return ioctl_fiemap(filp, arg);
 
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 149bec8..d48f19c 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -230,6 +230,7 @@ struct fsxattr {
 #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
 #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 #define FITRIM		_IOWR('X', 121, struct fstrim_range)	
/* Trim */
+#define FIGETFROZEN	_IOWR('X', 122, int)	/* Frozen
status */
 #define FICLONE		_IOW(0x94, 9, int)
 #define FICLONERANGE	_IOW(0x94, 13, struct file_clone_range)
 #define FIDEDUPERANGE	_IOWR(0x94, 54, struct file_dedupe_range)
-- 
2.8.0

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Mateusz Guzik]

On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
> This lets userland get the filesystem freezing status, aka whether the
> filesystem is frozen or not. This is so that an application can know if
> it should freeze the filesystem or if it isn't necessary when taking a
> snapshot.

The feature may be useful in general, I don't know.

However, I'm confused why programs would depend on it. If you froze
a particular subsystem, you don't have to check. If you did not, what
prevents whoever originaly froze it from unfreezing as you access it?

As such, maybe the feature you are looking for would count how many
times the fs is frozen.

-- 
Mateusz Guzik

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Dave Chinner]

On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
> This lets userland get the filesystem freezing status, aka whether the
> filesystem is frozen or not. This is so that an application can know if
> it should freeze the filesystem or if it isn't necessary when taking a
> snapshot.

freezing nests, so there is no reason for avoiding a freeze when
doing a snapshot. Indeed, if you don't wrap freeze/thaw around a
snapshot, then if the fs is thawed while the snapshot is in progress
then you are going to get a corrupt snapshot....

And, besides, polling for frozenness from userspace is inherently
racy - by the time the syscall returns, the information may be
incorrect, so you can't rely on it for decision making purposes in
userspace.

> +static int ioctl_fsgetfrozen(struct file *filp)
> +{
> + struct super_block *sb = file_inode(filp)->i_sb;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + return sb->s_writers.frozen;

This makes the internal freeze implementation states part of the
userspace ABI.  This needs an API that is separate from the internal
implementation...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Florian Margaine]

On Fri, 2016-04-15 at 12:17 +1000, Dave Chinner wrote:
> On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
> > This lets userland get the filesystem freezing status, aka whether
> > the
> > filesystem is frozen or not. This is so that an application can
> > know if
> > it should freeze the filesystem or if it isn't necessary when
> > taking a
> > snapshot.
> 
> freezing nests, so there is no reason for avoiding a freeze when
> doing a snapshot. Indeed, if you don't wrap freeze/thaw around a
> snapshot, then if the fs is thawed while the snapshot is in progress
> then you are going to get a corrupt snapshot....
> 
> And, besides, polling for frozenness from userspace is inherently
> racy - by the time the syscall returns, the information may be
> incorrect, so you can't rely on it for decision making purposes in
> userspace.

The example I have is mostly about unfreezing. If I freeze, make a
snapshot, then unfreeze. If my program crashes after the
snapshot/before unfreezing, I'll be left with a frozen filesystem,
which is undesirable, at best.

One way to mitigate this is to keep the state of the snapshot, but it
seems silly to keep this information separately (e.g. in a database)
when the kernel has it available.

> 
> > +static int ioctl_fsgetfrozen(struct file *filp)
> > +{
> > + struct super_block *sb = file_inode(filp)->i_sb;
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return -EPERM;
> > +
> > + return sb->s_writers.frozen;
> 
> This makes the internal freeze implementation states part of the
> userspace ABI.  This needs an API that is separate from the internal
> implementation...

I'm not sure I understand, do you mean it should be e.g.:

return sb->s_writers.frozen ? 1 : 0;

?

> 
> Cheers,
> 
> Dave.

Regards,
Florian

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Jan Kara]

On Sat 16-04-16 14:18:19, Florian Margaine wrote:
> On Fri, 2016-04-15 at 12:17 +1000, Dave Chinner wrote:
> > On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
> > > This lets userland get the filesystem freezing status, aka whether
> > > the
> > > filesystem is frozen or not. This is so that an application can
> > > know if
> > > it should freeze the filesystem or if it isn't necessary when
> > > taking a
> > > snapshot.
> > 
> > freezing nests, so there is no reason for avoiding a freeze when
> > doing a snapshot. Indeed, if you don't wrap freeze/thaw around a
> > snapshot, then if the fs is thawed while the snapshot is in progress
> > then you are going to get a corrupt snapshot....
> > 
> > And, besides, polling for frozenness from userspace is inherently
> > racy - by the time the syscall returns, the information may be
> > incorrect, so you can't rely on it for decision making purposes in
> > userspace.
> 
> The example I have is mostly about unfreezing. If I freeze, make a
> snapshot, then unfreeze. If my program crashes after the
> snapshot/before unfreezing, I'll be left with a frozen filesystem,
> which is undesirable, at best.
> 
> One way to mitigate this is to keep the state of the snapshot, but it
> seems silly to keep this information separately (e.g. in a database)
> when the kernel has it available.

Well, administrator can always unfreeze manually if he sees some glitch
like you described above has happened. And as Dave said, the interface you
propose is racy - how would you recognize the following two situations?

1) Your app has crashed with frozen fs.

2) Some other application is just snapshotting the fs and will unfreeze it
eventually.

In the first case you need additional thaw, in the second case you must not
thaw the fs. That's why we kept the decision about thawing the fs to the
administrator...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Eric Sandeen]

On 4/14/16 10:17 PM, Dave Chinner wrote:
> On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
>> This lets userland get the filesystem freezing status, aka whether the
>> filesystem is frozen or not. This is so that an application can know if
>> it should freeze the filesystem or if it isn't necessary when taking a
>> snapshot.
> 
> freezing nests, so there is no reason for avoiding a freeze when
> doing a snapshot. 

Sadly, no:

# xfs_freeze -f /mnt/test
# xfs_freeze -f /mnt/test
xfs_freeze: cannot freeze filesystem at /mnt/test: Device or resource busy

It used to, but it was broken^Wchanged quite some time ago.

> Indeed, if you don't wrap freeze/thaw around a
> snapshot, then if the fs is thawed while the snapshot is in progress
> then you are going to get a corrupt snapshot....

Yep.

IMHO what really needs to happen is to fix freeze to allow nesting
again.

A way to query freeze state might be nice, I think, but yeah, it's
racy, so you can't depend on it - but it might be useful in the "huh,
IO is failing, what's going on?  Oh, it's frozen, ok" scenario...

But if you want to be sure your snapshot is OK even while others are
running concurrent snapshots, we need nested freezes to work again.

-Eric

> And, besides, polling for frozenness from userspace is inherently
> racy - by the time the syscall returns, the information may be
> incorrect, so you can't rely on it for decision making purposes in
> userspace.
> 
>> +static int ioctl_fsgetfrozen(struct file *filp)
>> +{
>> + struct super_block *sb = file_inode(filp)->i_sb;
>> +
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + return sb->s_writers.frozen;
> 
> This makes the internal freeze implementation states part of the
> userspace ABI.  This needs an API that is separate from the internal
> implementation...
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Florian Margaine]

On Mon, 2016-04-18 at 11:20 -0400, Eric Sandeen wrote:
> 
> On 4/14/16 10:17 PM, Dave Chinner wrote:
> > On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
> > > This lets userland get the filesystem freezing status, aka
> > > whether the
> > > filesystem is frozen or not. This is so that an application can
> > > know if
> > > it should freeze the filesystem or if it isn't necessary when
> > > taking a
> > > snapshot.
> > 
> > freezing nests, so there is no reason for avoiding a freeze when
> > doing a snapshot. 
> 
> Sadly, no:
> 
> # xfs_freeze -f /mnt/test
> # xfs_freeze -f /mnt/test
> xfs_freeze: cannot freeze filesystem at /mnt/test: Device or resource
> busy
> 
> It used to, but it was broken^Wchanged quite some time ago.

I guess I can provide a patch for this. Silently making it a no-op if
the FS is already frozen in ioctl_fsfreeze() should be good enough?

> 
> > Indeed, if you don't wrap freeze/thaw around a
> > snapshot, then if the fs is thawed while the snapshot is in
> > progress
> > then you are going to get a corrupt snapshot....
> 
> Yep.
> 
> IMHO what really needs to happen is to fix freeze to allow nesting
> again.
> 
> A way to query freeze state might be nice, I think, but yeah, it's
> racy, so you can't depend on it - but it might be useful in the "huh,
> IO is failing, what's going on?  Oh, it's frozen, ok" scenario...

I guess my original use case was a bit contrived, but that or simply
monitoring would be glad to have this method, indeed.

> 
> But if you want to be sure your snapshot is OK even while others are
> running concurrent snapshots, we need nested freezes to work again.
> 
> -Eric
> 
> > And, besides, polling for frozenness from userspace is inherently
> > racy - by the time the syscall returns, the information may be
> > incorrect, so you can't rely on it for decision making purposes in
> > userspace.
> > 
> > > +static int ioctl_fsgetfrozen(struct file *filp)
> > > +{
> > > + struct super_block *sb = file_inode(filp)->i_sb;
> > > +
> > > + if (!capable(CAP_SYS_ADMIN))
> > > + return -EPERM;
> > > +
> > > + return sb->s_writers.frozen;
> > 
> > This makes the internal freeze implementation states part of the
> > userspace ABI.  This needs an API that is separate from the
> > internal
> > implementation...
> > 
> > Cheers,
> > 
> > Dave.
> > 

Regards,
Florian

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Eric Sandeen]

On 4/18/16 1:20 PM, Florian Margaine wrote:
> On Mon, 2016-04-18 at 11:20 -0400, Eric Sandeen wrote:
>> > 
>> > On 4/14/16 10:17 PM, Dave Chinner wrote:
>>> > > On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
>>>> > > > This lets userland get the filesystem freezing status, aka
>>>> > > > whether the
>>>> > > > filesystem is frozen or not. This is so that an application can
>>>> > > > know if
>>>> > > > it should freeze the filesystem or if it isn't necessary when
>>>> > > > taking a
>>>> > > > snapshot.
>>> > > 
>>> > > freezing nests, so there is no reason for avoiding a freeze when
>>> > > doing a snapshot. 
>> > 
>> > Sadly, no:
>> > 
>> > # xfs_freeze -f /mnt/test
>> > # xfs_freeze -f /mnt/test
>> > xfs_freeze: cannot freeze filesystem at /mnt/test: Device or resource
>> > busy
>> > 
>> > It used to, but it was broken^Wchanged quite some time ago.
> I guess I can provide a patch for this. Silently making it a no-op if
> the FS is already frozen in ioctl_fsfreeze() should be good enough?

That doesn't work, because you can go:

Process A: Freeze
Process A: Start snapshotting
Process B: Freeze; already frozen, no-op
Process B: Start snapshotting
Process A: Snapshot done, unfreeze
Process B: Now B is snapshotting an unfrozen filesystem

Freeze needs to be nested so that two freeze calls require two thaw
calls to make the filesystem active again, so that any given process
calling freeze can be reasonably sure that it will stay frozen until
it calls unfreeze at some later point.

-Eric

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Dave Chinner]

On Mon, Apr 18, 2016 at 11:20:22AM -0400, Eric Sandeen wrote:
> 
> 
> On 4/14/16 10:17 PM, Dave Chinner wrote:
> > On Thu, Apr 14, 2016 at 09:57:07AM +0200, Florian Margaine wrote:
> >> This lets userland get the filesystem freezing status, aka whether the
> >> filesystem is frozen or not. This is so that an application can know if
> >> it should freeze the filesystem or if it isn't necessary when taking a
> >> snapshot.
> > 
> > freezing nests, so there is no reason for avoiding a freeze when
> > doing a snapshot. 
> 
> Sadly, no:
> 
> # xfs_freeze -f /mnt/test
> # xfs_freeze -f /mnt/test
> xfs_freeze: cannot freeze filesystem at /mnt/test: Device or resource busy
> 
> It used to, but it was broken^Wchanged quite some time ago.

Ugh. Block device freeze nesting still works (i.e. freeze_bdev, as
snapshots from DM would use), but I didn't realise (or had
forgetten) that superblock level freeze nesting had been removed...

> > Indeed, if you don't wrap freeze/thaw around a
> > snapshot, then if the fs is thawed while the snapshot is in progress
> > then you are going to get a corrupt snapshot....
> 
> Yep.
> 
> IMHO what really needs to happen is to fix freeze to allow nesting
> again.

Probably. I quick dig shows nesting was intentionally broken more
than 5 years ago in making the freeze ioctl work on btrfs. 

commit 18e9e5104fcd9a973ffe3eed3816c87f2a1b6cd2
Author: Josef Bacik <josef@redhat.com>
Date:   Tue Mar 23 10:34:56 2010 -0400

    Introduce freeze_super and thaw_super for the fsfreeze ioctl
.....
    The only new gotcha is multiple calls to the fsfreeze ioctl will return EBUSY if
    the fs is already frozen.  I thought this was a better solution than adding a
    freeze counter to the super_block, but if everybody hates this idea I'm open to
    suggestions.  Thanks,
....

Not sure many people noticed that at the time....

> A way to query freeze state might be nice, I think, but yeah, it's
> racy, so you can't depend on it - but it might be useful in the "huh,
> IO is failing, what's going on?  Oh, it's frozen, ok" scenario...

So maybe we should just add the frozen state to /proc/self/mountinfo
or something similar, then people who think it matters can shoot
themselves in the foot all they want without us needing to care
about it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Florian Margaine]

On Tue, Apr 19, 2016 at 1:06 AM, Dave Chinner <david@fromorbit.com> wrote:
>> A way to query freeze state might be nice, I think, but yeah, it's
>> racy, so you can't depend on it - but it might be useful in the "huh,
>> IO is failing, what's going on?  Oh, it's frozen, ok" scenario...
>
> So maybe we should just add the frozen state to /proc/self/mountinfo
> or something similar, then people who think it matters can shoot
> themselves in the foot all they want without us needing to care
> about it.

Sorry if it's a basic question, but what is the difference between
/proc/self/mountinfo and providing an ioctl call?

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com


Regards,
Florian

Re: [PATCH] fs: add the FIGETFROZEN ioctl call

[Dave Chinner]

On Fri, Apr 22, 2016 at 11:53:48PM +0200, Florian Margaine wrote:
> On Tue, Apr 19, 2016 at 1:06 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> A way to query freeze state might be nice, I think, but yeah, it's
> >> racy, so you can't depend on it - but it might be useful in the "huh,
> >> IO is failing, what's going on?  Oh, it's frozen, ok" scenario...
> >
> > So maybe we should just add the frozen state to /proc/self/mountinfo
> > or something similar, then people who think it matters can shoot
> > themselves in the foot all they want without us needing to care
> > about it.
> 
> Sorry if it's a basic question, but what is the difference between
> /proc/self/mountinfo and providing an ioctl call?

Simply this:

$ grep xfs /proc/self/mountinfo
22 0 9:0 / / rw,relatime - xfs /dev/block/9:0 rw,attr2,inode64,sunit=1024,swidth=2048,noquota

i.e. no need for a custom binary to query state in diagnostic
situations where it might matter. And it shows up in things like SOS
reports that distro's gather when users report issues.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com