[PATCH] COW and checksumming ioctls

[PATCH] COW and checksumming ioctls

[Mingming]

COW and checksumming is enabled by default, and could be disable with
mount option.  These new ioctls below let a user application turn on/off
copy-on-write and checksumming on a per file basis.

Signed-off-by: Mingming Cao <cmm@us.ibm.com>

diff -r 79b81fd6dd78 ioctl.c
--- a/ioctl.c	Thu Jun 12 14:46:17 2008 -0400
+++ b/ioctl.c	Thu Jun 19 15:59:02 2008 -0700
@@ -471,6 +471,55 @@ static int btrfs_ioctl_defrag(struct fil
 
 	return 0;
 }
+
+static int btrfs_ioctl_disable_datacow(struct file *file)
+{
+	struct inode *inode = fdentry(file)->d_inode;
+
+	if ((inode->i_mode & S_IFMT) == S_IFREG) {
+		mutex_lock(&inode->i_mutex);
+		btrfs_set_flag(inode, NODATACOW);
+		mutex_unlock(&inode->i_mutex);
+	}
+
+	return 0;
+}
+
+static int btrfs_ioctl_enable_datacow(struct file *file)
+{
+	struct inode *inode = fdentry(file)->d_inode;
+
+	if ((inode->i_mode & S_IFMT) == S_IFREG) {
+		mutex_lock(&inode->i_mutex);
+		btrfs_clear_flag(inode, NODATACOW);
+		mutex_unlock(&inode->i_mutex);
+	}
+
+	return 0;
+}
+
+static int btrfs_ioctl_disable_datasum(struct file *file)
+{
+	struct inode *inode = fdentry(file)->d_inode;
+
+	mutex_lock(&inode->i_mutex);
+	btrfs_set_flag(inode, NODATASUM);
+	mutex_unlock(&inode->i_mutex);
+
+	return 0;
+}
+
+static int btrfs_ioctl_enable_datasum(struct file *file)
+{
+	struct inode *inode = fdentry(file)->d_inode;
+
+	mutex_lock(&inode->i_mutex);
+	btrfs_clear_flag(inode, NODATASUM);
+	mutex_unlock(&inode->i_mutex);
+
+	return 0;
+}
+
 
 long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
 {
@@ -775,6 +824,14 @@ long btrfs_ioctl(struct file *file, unsi
 		return btrfs_ioctl_trans_start(file);
 	case BTRFS_IOC_TRANS_END:
 		return btrfs_ioctl_trans_end(file);
+	case BTRFS_IOC_NODATACOW:
+		return btrfs_ioctl_disable_datacow(file);
+	case BTRFS_IOC_DATACOW:
+		return btrfs_ioctl_enable_datacow(file);
+	case BTRFS_IOC_NODATASUM:
+		return btrfs_ioctl_disable_datasum(file);
+	case BTRFS_IOC_DATASUM:
+		return btrfs_ioctl_enable_datasum(file);
 	case BTRFS_IOC_SYNC:
 		btrfs_sync_fs(file->f_dentry->d_sb, 1);
 		return 0;
diff -r 79b81fd6dd78 ioctl.h
--- a/ioctl.h	Thu Jun 12 14:46:17 2008 -0400
+++ b/ioctl.h	Thu Jun 19 15:32:40 2008 -0700
@@ -51,5 +51,8 @@ struct btrfs_ioctl_vol_args {
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_BALANCE _IOW(BTRFS_IOCTL_MAGIC, 12, \
 				   struct btrfs_ioctl_vol_args)
-
+#define BTRFS_IOC_NODATACOW _IO(BTRFS_IOCTL_MAGIC, 13)
+#define BTRFS_IOC_DATACOW _IO(BTRFS_IOCTL_MAGIC, 14)
+#define BTRFS_IOC_NODATASUM _IO(BTRFS_IOCTL_MAGIC, 15)
+#define BTRFS_IOC_DATASUM _IO(BTRFS_IOCTL_MAGIC, 16)
 #endif

Re: [PATCH] COW and checksumming ioctls

[Zach Brown]

> +#define BTRFS_IOC_NODATACOW _IO(BTRFS_IOCTL_MAGIC, 13)
> +#define BTRFS_IOC_DATACOW _IO(BTRFS_IOCTL_MAGIC, 14)
> +#define BTRFS_IOC_NODATASUM _IO(BTRFS_IOCTL_MAGIC, 15)
> +#define BTRFS_IOC_DATASUM _IO(BTRFS_IOCTL_MAGIC, 16)

Hmm.  Do we really want 4 different ioctl commands to turn 2 features on
and off?  Surely we could have 1 ioctl which updates a bitfield?  Or a
ioctl that takes an explicit feature enum argument and a boolean which
indicates that it should be enabled or not?

(And is ioctl the right interface for this?  maybe it should be xattr
ops in some defined btrfs string namespace?  I'm just making this up.  I
feel the the lack of a single comment in the patch, while in keeping
with the existing precedent in btrfs, leaves a lot of room for wild
speculation :))

- z

Re: [PATCH] COW and checksumming ioctls

[Joe Peterson]

Zach Brown wrote:
> Hmm.  Do we really want 4 different ioctl commands to turn 2 features on
> and off?  Surely we could have 1 ioctl which updates a bitfield?  Or a
> ioctl that takes an explicit feature enum argument and a boolean which
> indicates that it should be enabled or not?
> 
> (And is ioctl the right interface for this?  maybe it should be xattr
> ops in some defined btrfs string namespace?  I'm just making this up.  I
> feel the the lack of a single comment in the patch, while in keeping
> with the existing precedent in btrfs, leaves a lot of room for wild
> speculation :))

Beyond that, is it really desirable to be able to turn off
checksumming/COW per file?  Or even to be able to do it so easily?

I feel that checksumming is an extremely important feature, and it
scares me even to be able to disable it at all (what if someone
mistakenly does it, and then all of the data is unintentionally
vulnerable?).  But at least if it is only a mount option, the mistake
would have to be at as system level and would be harder to do by accident.

If someone were to turn off checksumming and then turn it back on, I
assume they would lose any continuity of protection that exists if it
stays on, so it would have to be a major and important decision in an
existing filesystem.

						-Joe

Re: [PATCH] COW and checksumming ioctls

[Josef Bacik]

On Fri, Jun 20, 2008 at 08:01:54AM -0600, Joe Peterson wrote:
> Zach Brown wrote:
> > Hmm.  Do we really want 4 different ioctl commands to turn 2 features on
> > and off?  Surely we could have 1 ioctl which updates a bitfield?  Or a
> > ioctl that takes an explicit feature enum argument and a boolean which
> > indicates that it should be enabled or not?
> > 
> > (And is ioctl the right interface for this?  maybe it should be xattr
> > ops in some defined btrfs string namespace?  I'm just making this up.  I
> > feel the the lack of a single comment in the patch, while in keeping
> > with the existing precedent in btrfs, leaves a lot of room for wild
> > speculation :))
> 
> Beyond that, is it really desirable to be able to turn off
> checksumming/COW per file?  Or even to be able to do it so easily?
>

Database apps that do their own complicated stuff to make sure everything makes
it to the disk properly who dont want the extra overhead of checksumming.
 
> I feel that checksumming is an extremely important feature, and it
> scares me even to be able to disable it at all (what if someone
> mistakenly does it, and then all of the data is unintentionally
> vulnerable?).  But at least if it is only a mount option, the mistake
> would have to be at as system level and would be harder to do by accident.
> 

What if somebody logs in as root and does an rm -rf?  I'm not thinking that
running the command to disable checksumming on a file will be something that
gets run often by accident, but even if it does the mantra of linux in general
has never been "dont let users do something because some idiot could screw it
all up."

> If someone were to turn off checksumming and then turn it back on, I
> assume they would lose any continuity of protection that exists if it
> stays on, so it would have to be a major and important decision in an
> existing filesystem.
>

I don't really understand the above objection.  Checksumming doesn't make
everything magically protected, just makes it easier to catch when problems are
happening.  I don't see how having it off then turning it on would cause any
sorts of issues.

Josef

Re: [PATCH] COW and checksumming ioctls

[Joe Peterson]

Josef Bacik wrote:
> Database apps that do their own complicated stuff to make sure everything makes
> it to the disk properly who dont want the extra overhead of checksumming.

I do see the benefit of that, if indeed the DB does end-to-end checking.
 Then again, I could almost see that perhaps making the setting based on
something at the subvolume level (not per-file level) might be even
better for that case.

> What if somebody logs in as root and does an rm -rf?  I'm not thinking that
> running the command to disable checksumming on a file will be something that
> gets run often by accident, but even if it does the mantra of linux in general
> has never been "dont let users do something because some idiot could screw it
> all up."

Sure, that's true.  I am not saying that we need to protect users from
themselves, as long as there is a way to clearly see what the settings
are (so a user/admin can verify the state, e.g.).  I guess my main
concern would be if it is relatively easy for this to go completely
unnoticed.  Also, would it be only root who could change such a setting?

> I don't really understand the above objection.  Checksumming doesn't make
> everything magically protected, just makes it easier to catch when problems are
> happening.  I don't see how having it off then turning it on would cause any
> sorts of issues.

Well, with checksumming on, there is continuous protection.  And what I
mean by "protection" is peace-of-mind that the data integrity has been
checked (I don't necessarily mean protected from loss).  For me,
*knowing* that the data is bad is the most important thing, so it does
not silently propagate to backups, etc.

So, if a file, say, is "protected" by checksumming as it gets read,
changed, etc., there is a continuity of protection, and the user can
have confidence that the data read is the same as the data written.

On the other hand, if one turns off checksumming, there is then a break
in that continuity, so any further reads or writes will not be be
checked.  Even if the file is not written while not having checksumming
enabled, but then it is re-enabled, now there is no way to know that the
file's contents are the same as what they were before checksumming was
turned off - that confidence is lost.

						-Joe

Re: [PATCH] COW and checksumming ioctls

[Chris Mason]

On Fri, 2008-06-20 at 09:07 -0600, Joe Peterson wrote:
> Josef Bacik wrote:
> > Database apps that do their own complicated stuff to make sure everything makes
> > it to the disk properly who dont want the extra overhead of checksumming.
> 
> I do see the benefit of that, if indeed the DB does end-to-end checking.
>  Then again, I could almost see that perhaps making the setting based on
> something at the subvolume level (not per-file level) might be even
> better for that case.
> 

But, it'll be common to mix database files with files that you do want
checksummed in the same volume.  This is an admin level decision, and we
can easily provide knobs to turn it on/off.  So, I think the ioctl is
really important and we'll just document it as best we can.

Long term, these might end up getting folded into chattr, so I think
Zach has a good point about folding them into a single
change-file-attributes ioctl.

-chris

Re: [PATCH] COW and checksumming ioctls

[Joe Peterson]

Chris Mason wrote:
> But, it'll be common to mix database files with files that you do want
> checksummed in the same volume.  This is an admin level decision, and we
> can easily provide knobs to turn it on/off.  So, I think the ioctl is
> really important and we'll just document it as best we can.

I do see now why this ability is desirable, especially if there is a way
to see a file's checksum/COW settings (or better still, to query the
filesystem for which files have certain settings).  My initial concern
was that an admin or user may not realize (or forget) a file was left in
an "unprotected" state, but yes, I'm sure there are ways to mitigate
that possibility.

						-Joe

Re: [PATCH] COW and checksumming ioctls

[jim owens]

Joe Peterson wrote:

>  Then again, I could almost see that perhaps making the setting based on
> something at the subvolume level (not per-file level) might be even
> better for that case. 
...
> Sure, that's true.  I am not saying that we need to protect users from
> themselves, as long as there is a way to clearly see what the settings
> are (so a user/admin can verify the state, e.g.).  I guess my main
> concern would be if it is relatively easy for this to go completely
> unnoticed.  Also, would it be only root who could change such a setting?

It is important to have the per-file granularity.  I agree with
you that being able to apply this to "all members of this set",
where the "set" is whatever is practical for btrfs is a nice
ease-of-use feature.

The ability to do the change is probably based on "right to
change file attributes", which depends on security policy,
but it should not be designed as a root-only restriction.

As I think someone already said (or hinted), a feature-lockout
can be designed so the admnin can do that on some "set" for
those who are paranoid.

Again for those who are paranoid, making these auditable events
(I'm probably not using the right linux term) solves the need
to know something like checksum-off has occured.

jim

Re: [PATCH] COW and checksumming ioctls

[Joe Peterson]

jim owens wrote:
> As I think someone already said (or hinted), a feature-lockout
> can be designed so the admnin can do that on some "set" for
> those who are paranoid.

That would be a nice feature.  Maybe even a mount option could be
available to "disable the disabling" of checksumming/COW for cases in
which the admin decides users should not be changing these attributes
for a particular filesystem.

> Again for those who are paranoid, making these auditable events
> (I'm probably not using the right linux term) solves the need
> to know something like checksum-off has occured.

Yes, that reminds me of ZFS's logging facility, in which case an admin
can check to see all operations ever done to the filesystem.  When
someone is managing a bunch of computers and/or filesystems (making it
easy to get forgetful), it is always nice to be able to go back and ask
the system what admin options were used as a sanity check.  I think
that's a cool idea.

						-Joe

Re: [PATCH] COW and checksumming ioctls

[Christoph Hellwig]

On Thu, Jun 19, 2008 at 10:23:17PM -0700, Zach Brown wrote:
> 
> > +#define BTRFS_IOC_NODATACOW _IO(BTRFS_IOCTL_MAGIC, 13)
> > +#define BTRFS_IOC_DATACOW _IO(BTRFS_IOCTL_MAGIC, 14)
> > +#define BTRFS_IOC_NODATASUM _IO(BTRFS_IOCTL_MAGIC, 15)
> > +#define BTRFS_IOC_DATASUM _IO(BTRFS_IOCTL_MAGIC, 16)
> 
> Hmm.  Do we really want 4 different ioctl commands to turn 2 features on
> and off?  Surely we could have 1 ioctl which updates a bitfield?  Or a
> ioctl that takes an explicit feature enum argument and a boolean which
> indicates that it should be enabled or not?

The best interface for per-file boolean flags
FS_IOC_SETFLAGS/FS_IOC_GETFLAGS ioctls which would fir this really
nicely.

> (And is ioctl the right interface for this?  maybe it should be xattr
> ops in some defined btrfs string namespace?  I'm just making this up.  I
> feel the the lack of a single comment in the patch, while in keeping
> with the existing precedent in btrfs, leaves a lot of room for wild
> speculation :))

Synthetic xattrs are a really utterly horrible interface.  Xattrs on
disk are nice and simple, but the Linux invention of making some up
on the fly, starting with the Posix ACL interface makes the
implementation not just utterly complicated but also confuses backup
programs.

Re: [PATCH] COW and checksumming ioctls

[Chris Mason]

On Sat, 2008-06-21 at 03:27 -0400, Christoph Hellwig wrote:
> On Thu, Jun 19, 2008 at 10:23:17PM -0700, Zach Brown wrote:
> > 
> > > +#define BTRFS_IOC_NODATACOW _IO(BTRFS_IOCTL_MAGIC, 13)
> > > +#define BTRFS_IOC_DATACOW _IO(BTRFS_IOCTL_MAGIC, 14)
> > > +#define BTRFS_IOC_NODATASUM _IO(BTRFS_IOCTL_MAGIC, 15)
> > > +#define BTRFS_IOC_DATASUM _IO(BTRFS_IOCTL_MAGIC, 16)
> > 
> > Hmm.  Do we really want 4 different ioctl commands to turn 2 features on
> > and off?  Surely we could have 1 ioctl which updates a bitfield?  Or a
> > ioctl that takes an explicit feature enum argument and a boolean which
> > indicates that it should be enabled or not?
> 
> The best interface for per-file boolean flags
> FS_IOC_SETFLAGS/FS_IOC_GETFLAGS ioctls which would fir this really
> nicely.
> 
> > (And is ioctl the right interface for this?  maybe it should be xattr
> > ops in some defined btrfs string namespace?  I'm just making this up.  I
> > feel the the lack of a single comment in the patch, while in keeping
> > with the existing precedent in btrfs, leaves a lot of room for wild
> > speculation :))
> 
> Synthetic xattrs are a really utterly horrible interface.  Xattrs on
> disk are nice and simple, but the Linux invention of making some up
> on the fly, starting with the Posix ACL interface makes the
> implementation not just utterly complicated but also confuses backup
> programs.

The idea is that backup programs already know how to do xattrs and can
easily be changed to preserve them.  Every ioctl interface we
create/make up has to be handed coded into the backup program.

I know xattrs are ugly, but we need to weigh the cost of the perfect
interface with the availability of a common one.  Dave Chinner had
talked about using xattrs to control file behavior in XFS as well, not
sure if that ever happened.

-chris

Re: [PATCH] COW and checksumming ioctls

[Joshua J. Berry]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

On Sunday 22 June 2008 07:10:06 Chris Mason wrote:
> On Sat, 2008-06-21 at 03:27 -0400, Christoph Hellwig wrote:
> > Synthetic xattrs are a really utterly horrible interface.  Xattrs on
> > disk are nice and simple, but the Linux invention of making some up
> > on the fly, starting with the Posix ACL interface makes the
> > implementation not just utterly complicated but also confuses backup
> > programs.
>
> The idea is that backup programs already know how to do xattrs and can
> easily be changed to preserve them.  Every ioctl interface we
> create/make up has to be handed coded into the backup program.
>
> I know xattrs are ugly, but we need to weigh the cost of the perfect
> interface with the availability of a common one.  Dave Chinner had
> talked about using xattrs to control file behavior in XFS as well, not
> sure if that ever happened.

+1 for xattrs.  From a user perspective, they're simpler than futzing around 
with special btrfs programs to set/unset attributes, and as Chris points out, 
having backup programs pick them up is a good thing.

If virtual xattrs are so complicated/messy to implement, perhaps they should 
be made simpler.  I honestly can't think of a better interface for something 
like this.

-- Josh


-- 
Joshua J. Berry

"I haven't lost my mind -- it's backed up on tape somewhere."
    -- /usr/games/fortune


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] COW and checksumming ioctls

[Christoph Hellwig]

On Sun, Jun 22, 2008 at 10:10:06AM -0400, Chris Mason wrote:
> The idea is that backup programs already know how to do xattrs and can
> easily be changed to preserve them.  Every ioctl interface we
> create/make up has to be handed coded into the backup program.

Actually they don't.  Because of the mess we've created with the
xattr namespaces and the way e.g. ACLs are mapped into two of them
they will need to special case the interesting non-user attributes.

While sane backup applications already know about the inode flag word
because it contains lots of useful information for the existing
filesystems.

> I know xattrs are ugly, but we need to weigh the cost of the perfect
> interface with the availability of a common one.  Dave Chinner had
> talked about using xattrs to control file behavior in XFS as well, not
> sure if that ever happened.

I think I've successfully talked him out of it :)