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

From: Florian Margaine <florian@margaine.com>
To: Eric Sandeen <sandeen@sandeen.net>, Dave Chinner <david@fromorbit.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Jeff Layton <jlayton@poochiereds.net>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fs: add the FIGETFROZEN ioctl call
Date: Mon, 18 Apr 2016 19:20:03 +0200	[thread overview]
Message-ID: <1461000003.6411.1.camel@margaine.com> (raw)
In-Reply-To: <5714FB36.6020704@sandeen.net>

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