After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 626497 - Btrfs clone/reflink ioctl support in g_local_file_copy
Btrfs clone/reflink ioctl support in g_local_file_copy
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
: 659480 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-08-10 04:36 UTC by Nirbheek Chauhan
Modified: 2013-01-25 15:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add btrfs clone ioctl support to GFile via g_local_file_copy (6.91 KB, patch)
2010-08-10 04:36 UTC, Nirbheek Chauhan
none Details | Review
Re-use sanity checks from open_source_for_copy() and g_local_file_replace() (7.71 KB, patch)
2010-08-29 12:59 UTC, Nirbheek Chauhan
needs-work Details | Review
Btrfs reflink inside g_local_file_copy (patch with hack) (8.40 KB, patch)
2013-01-05 11:23 UTC, Nirbheek Chauhan
reviewed Details | Review
Btrfs reflink inside file_copy_fallback for GFile (6.11 KB, patch)
2013-01-05 16:17 UTC, Nirbheek Chauhan
committed Details | Review

Description Nirbheek Chauhan 2010-08-10 04:36:47 UTC
Created attachment 167473 [details] [review]
Add btrfs clone ioctl support to GFile via g_local_file_copy

The attached patch adds support for the btrfs "clone" ioctl which makes Copy-on-Write reflinks, resulting in cheap O(1) copies when source/destination are on the same filesystem. The ioctl itself is quite straightforward, and GNU coreutils has had support since 7.5 (--reflink=auto --sparse=auto).

The ioctl only operates on regular files and symlinks, and always follows symlinks; checks have been added accordingly.

This patch would be very useful for everyone who uses btrfs filesystems (Meego folks for instance). On systems that don't have btrfs, or if the the source is not on a btrfs filesystem, the ioctl returns EINVAL, and the fallback code is triggered. Hence this will cause no problems for non-btrfs users.
Comment 1 Nirbheek Chauhan 2010-08-29 12:59:34 UTC
Created attachment 168997 [details] [review]
Re-use sanity checks from open_source_for_copy() and g_local_file_replace()

This update does the following:

* Fixes a problem with nautilus copying directories by raising G_IO_ERROR_WOULD_RECURSE instead of G_IO_ERROR_IS_DIRECTORY when source is a directory
* Re-uses the checks in open_source_for_copy() and g_local_file_replace()
* Adds a new gfileprivate.h header to declare the open_source_for_copy symbol from gfile.c
* Add a comment explaining the capabilities of the clone ioctl()
Comment 2 Matthias Clasen 2012-08-17 05:53:30 UTC
*** Bug 659480 has been marked as a duplicate of this bug. ***
Comment 3 Colin Walters 2012-08-25 17:52:06 UTC
Review of attachment 168997 [details] [review]:

Thanks for the patch!

::: gio/gfileprivate.h
@@ +28,3 @@
+
+G_BEGIN_DECLS
+

Should have G_GNUC_INTERNAL here.  We tend to use the g_ prefix even for internal functions, and use that macro to keep private symbols private.

::: gio/glocalfile.c
@@ +50,3 @@
+#include <sys/ioctl.h>
+#define BTRFS_IOCTL_MAGIC 0x94
+#define BTRFS_IOC_CLONE _IOW(BTRFS_IOCTL_MAGIC, 9, int)

Add a comment link to the kernel header please.  Like

/* See fs/btrfs/ioctl.h */

@@ +2195,3 @@
 		   GError                **error)
 {
+#if HAVE_SYS_IOCTL_H

#ifdef, not #if

@@ +2215,3 @@
+  info = g_file_query_info (source, G_FILE_ATTRIBUTE_STANDARD_TYPE,
+		  	    G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS,
+			    cancellable, error);

It'd be more efficient to call open_source_for_copy() here first, handling errors, and get the file info from that; basically the equivalent of calling open() + fstat(), rather than stat() + open()

@@ +2222,3 @@
+  source_size = g_file_info_get_size (info);
+
+  if (source_type & G_FILE_TYPE_SPECIAL)

source_type == , not &

@@ +2231,3 @@
+    }
+  /* Btrfs clone ioctl follows symlinks */
+  else if ((source_type & G_FILE_TYPE_SYMBOLIC_LINK) &&

source_type ==

@@ +2247,3 @@
+			     _("Target file exists"));
+	return FALSE;
+      }

You need to g_clear_error() here - otherwise error will still be set.

@@ +2287,3 @@
+	/* Most probably something odd happened,
+	 * or this is not a btrfs filesystem */
+	goto fallback;

Er...we should assume EINVAL means it's not supported.  Anything else "odd" (like say, ENOSPC), should be an error.

@@ +2299,3 @@
+    progress_callback (source_size, source_size, progress_callback_data);
+
+  return TRUE;

This function leaks all over the place.  You need to g_object_unref() all of "info", "in", and "out".
Comment 4 Nirbheek Chauhan 2013-01-05 11:23:56 UTC
Created attachment 232819 [details] [review]
Btrfs reflink inside g_local_file_copy (patch with hack)

(In reply to comment #3)
> Review of attachment 168997 [details] [review]:
> 
> Thanks for the patch!
> 

Thanks for reviewing! I was very surprised to see a review for my patch after so long. :)

And… sorry about the delay in replying. I was really occupied in the last few months. :(

> ::: gio/gfileprivate.h
> @@ +28,3 @@
> +
> +G_BEGIN_DECLS
> +
> 
> Should have G_GNUC_INTERNAL here.  We tend to use the g_ prefix even for
> internal functions, and use that macro to keep private symbols private.
> 

Fixed!

> ::: gio/glocalfile.c
> @@ +50,3 @@
> +#include <sys/ioctl.h>
> +#define BTRFS_IOCTL_MAGIC 0x94
> +#define BTRFS_IOC_CLONE _IOW(BTRFS_IOCTL_MAGIC, 9, int)
> 
> Add a comment link to the kernel header please.  Like
> 
> /* See fs/btrfs/ioctl.h */
> 

Fixed!

> @@ +2195,3 @@
>             GError                **error)
>  {
> +#if HAVE_SYS_IOCTL_H
> 
> #ifdef, not #if
> 

Fixed!

> @@ +2215,3 @@
> +  info = g_file_query_info (source, G_FILE_ATTRIBUTE_STANDARD_TYPE,
> +                  G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS,
> +                cancellable, error);
> 
> It'd be more efficient to call open_source_for_copy() here first, handling
> errors, and get the file info from that; basically the equivalent of calling
> open() + fstat(), rather than stat() + open()
> 

I did this, and then realised that we need to know what the source is before attempting to open it for copying. For example, if it's a dangling symlink, the fallback code copies it as it is, but open_source_for_copy() will fail on it and error out instead of triggering the fallback code like it should.

The structure of this code is quite similar to file_copy_fallback() in this sense.

> @@ +2222,3 @@
> +  source_size = g_file_info_get_size (info);
> +
> +  if (source_type & G_FILE_TYPE_SPECIAL)
> 
> source_type == , not &
> 

Fixed!

> @@ +2231,3 @@
> +    }
> +  /* Btrfs clone ioctl follows symlinks */
> +  else if ((source_type & G_FILE_TYPE_SYMBOLIC_LINK) &&
> 
> source_type ==
> 

Ditto.

> @@ +2247,3 @@
> +                 _("Target file exists"));
> +    return FALSE;
> +      }
> 
> You need to g_clear_error() here - otherwise error will still be set.
> 

I changed the code to do a g_local_file_create if not overwriting, which should handle and propagate the error properly.

> @@ +2287,3 @@
> +    /* Most probably something odd happened,
> +     * or this is not a btrfs filesystem */
> +    goto fallback;
> 
> Er...we should assume EINVAL means it's not supported.  Anything else "odd"
> (like say, ENOSPC), should be an error.
> 

I added EINVAL to the checked list (returning NOT_SUPPORTED), but other errors might not be fatal, and so I think it's good to trigger the fallback code to try just in case. If it's ENOSPC or some other hard error, the fallback code should fail in the same way.

> @@ +2299,3 @@
> +    progress_callback (source_size, source_size, progress_callback_data);
> +
> +  return TRUE;
> 
> This function leaks all over the place.  You need to g_object_unref() all of
> "info", "in", and "out".

Oops! It looks like this patch shows my level of C knowledge at the time that I wrote it. ;)

--

After fixing all the problems, I noticed a problem, and the best solution I can think for it is to rewrite the patch to try the ioctl inside gio/gfile.c:file_copy_fallback().

The problem:

I was using g_local_file_replace() because the ioctl needs an empty destination file (because it needs an fd), and also to take advantage of the safe file replace logic inside g_local_file_replace.

However, if the ioctl fails, this will leave behind an empty file, and will cause the fallback copy (triggered by G_IO_ERROR_NOT_SUPPORTED) to fail if G_FILE_COPY_OVERWRITE is not set.

Using g_file_delete to remove that temporary file feels very hacky and wrong.

Moving the whole logic inside file_copy_fallback() would solve this problem, and would also remove a lot of duplicated checks and code. However, I'm not sure if that fits in with the layering inside gio.

What do you think?

I'll start working on a patch for file_copy_fallback() in the meantime. The attached patch does a g_file_delete for now.
Comment 5 Colin Walters 2013-01-05 12:08:09 UTC
(In reply to comment #4)

> Thanks for reviewing! I was very surprised to see a review for my patch after
> so long. :)

Following up with pings for patches after some time (an interval of a month is acceptable).

At the same time, GLib has gained a bit more of an active peer review process and pool of reviewers since you submitted this patch.
 
> I did this, and then realised that we need to know what the source is before
> attempting to open it for copying. For example, if it's a dangling symlink, the
> fallback code copies it as it is, but open_source_for_copy() will fail on it
> and error out instead of triggering the fallback code like it should.

Ah.  True.

> I added EINVAL to the checked list (returning NOT_SUPPORTED), but other errors
> might not be fatal,

Like what errors?

> and so I think it's good to trigger the fallback code to
> try just in case. If it's ENOSPC or some other hard error, the fallback code
> should fail in the same way.

Hmmm.  I'm not strictly opposed, but I'm curious if you have a specific scenario in mind where this would be necessary, or you're just saying it's a gut feeling.

> Oops! It looks like this patch shows my level of C knowledge at the time that I
> wrote it. ;)

That's OK, Rhythmbox's source is a testament to my C learning curve ;)
 
> --
> 
> After fixing all the problems, I noticed a problem, and the best solution I can
> think for it is to rewrite the patch to try the ioctl inside
> gio/gfile.c:file_copy_fallback().
> 
> The problem:
> 
> I was using g_local_file_replace() because the ioctl needs an empty destination
> file (because it needs an fd), and also to take advantage of the safe file
> replace logic inside g_local_file_replace.
> 
> However, if the ioctl fails, this will leave behind an empty file, and will
> cause the fallback copy (triggered by G_IO_ERROR_NOT_SUPPORTED) to fail if
> G_FILE_COPY_OVERWRITE is not set.
> 
> Using g_file_delete to remove that temporary file feels very hacky and wrong.

I don't see why - we do this in the g_file_replace() logic with the .goutputstream-XXXXXX tmpfile.  What's hacky?

(You should use unlink() directly though, not g_file_delete(), since you know it's a local file).
Comment 6 Colin Walters 2013-01-05 12:27:19 UTC
Review of attachment 232819 [details] [review]:

Some minor bits:

::: gio/gfileprivate.h
@@ +1,3 @@
+/* gio - glib input, output and streaming library
+ *
+ * copyright (c) 2006-2007 red hat, inc.

First: It's "Red Hat, Inc.", not "red hat, inc." - did this header get run through something that eats capital letters?

Second, you should really have yourself here, not Red Hat, since you wrote the only interesting parts of the header.

::: gio/glocalfile.c
@@ +2251,3 @@
+			   G_IO_ERROR_NOT_SUPPORTED,
+			   _("Can't copy special files"));
+      goto out;

Why isn't this one just "goto fallback;" too?

@@ +2308,3 @@
+	g_set_error_literal (error, G_IO_ERROR,
+			     G_IO_ERROR_NOT_SUPPORTED,
+			     _("Copy (reflink/clone) is not supported"));

You need to do:

ret = TRUE;
goto fallback;

in both of the cases above; otherwise we do the progress callback twice.  

Or, better: skip setting the gerror, and just "goto fallback;".  It's not like anyone will see the text anyways, the parent function will just key off the G_IO_ERROR_NOT_SUPPORTED code.
Comment 7 Nirbheek Chauhan 2013-01-05 12:29:48 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > I added EINVAL to the checked list (returning NOT_SUPPORTED), but other errors
> > might not be fatal,
> 
> Like what errors?
> 
> > and so I think it's good to trigger the fallback code to
> > try just in case. If it's ENOSPC or some other hard error, the fallback code
> > should fail in the same way.
> 
> Hmmm.  I'm not strictly opposed, but I'm curious if you have a specific
> scenario in mind where this would be necessary, or you're just saying it's a
> gut feeling.
> 

The problem is that there is no way to know what errno an ioctl() might set, and with Btrfs being in a state of flux, I think it would be nice to bullet-proof oneself against errors and changes in the filesystem code.

> > However, if the ioctl fails, this will leave behind an empty file, and will
> > cause the fallback copy (triggered by G_IO_ERROR_NOT_SUPPORTED) to fail if
> > G_FILE_COPY_OVERWRITE is not set.
> > 
> > Using g_file_delete to remove that temporary file feels very hacky and wrong.
> 
> I don't see why - we do this in the g_file_replace() logic with the
> .goutputstream-XXXXXX tmpfile.  What's hacky?
> 

What's hacky is that it relies on the internal implementation of gio/glocalfileoutputstream.c:handle_overwrite_open(), and if that changes, this code would suddenly be completely wrong.

We would completely avoid this by doing this work inside gio/gfile.c:file_copy_fallback(), since the fd would only be used once. As a bonus, we would also not call g_file_query_info multiple times and not open source/dest files multiple times.

This ioctl() might actually fit in with the splice code as well.

> (You should use unlink() directly though, not g_file_delete(), since you know
> it's a local file).

What about g_local_file_delete()? This might sound silly, but I tend to bypass gio/glib wrappers only as a last resort. :)
Comment 8 Nirbheek Chauhan 2013-01-05 12:37:20 UTC
(In reply to comment #6)
> Review of attachment 232819 [details] [review]:
> ::: gio/gfileprivate.h
> @@ +1,3 @@
> +/* gio - glib input, output and streaming library
> + *
> + * copyright (c) 2006-2007 red hat, inc.
> 
> First: It's "Red Hat, Inc.", not "red hat, inc." - did this header get run
> through something that eats capital letters?
> 

That's odd. I had simply copied gfile.h to create that header. I'll fix it.

> Second, you should really have yourself here, not Red Hat, since you wrote the
> only interesting parts of the header.
> 

I wasn't sure about the etiquette in doing that; I'll fix that as well. :) 

> ::: gio/glocalfile.c
> @@ +2251,3 @@
> +               G_IO_ERROR_NOT_SUPPORTED,
> +               _("Can't copy special files"));
> +      goto out;
> 
> Why isn't this one just "goto fallback;" too?
> 

Because fallback: is used only to set a generic G_IO_ERROR_NOT_SUPPORTED, and that snippet already does that with a better error message.

> @@ +2308,3 @@
> +    g_set_error_literal (error, G_IO_ERROR,
> +                 G_IO_ERROR_NOT_SUPPORTED,
> +                 _("Copy (reflink/clone) is not supported"));
> 
> You need to do:
> 
> ret = TRUE;
> goto fallback;
> 
> in both of the cases above; otherwise we do the progress callback twice.  
> 

I don't understand how that could happen. Won't that codepath lead to "goto out;", and exit the function with ret = FALSE?

> Or, better: skip setting the gerror, and just "goto fallback;".  It's not like
> anyone will see the text anyways, the parent function will just key off the
> G_IO_ERROR_NOT_SUPPORTED code.

I just thought it would aid in debugging in the future if this bit of extra information was passed on up the stack.
Comment 9 Colin Walters 2013-01-05 12:45:22 UTC
(In reply to comment #7)

> The problem is that there is no way to know what errno an ioctl() might set,
> and with Btrfs being in a state of flux, I think it would be nice to
> bullet-proof oneself against errors and changes in the filesystem code.

Ok.

> What's hacky is that it relies on the internal implementation of
> gio/glocalfileoutputstream.c:handle_overwrite_open(), and if that changes, this
> code would suddenly be completely wrong.

I guess, but this seems like a problem best addressed by test coverage (an open question here, since GLib presently relies on "make distcheck" run on developer laptops which may or may not be running btrfs).

> We would completely avoid this by doing this work inside
> gio/gfile.c:file_copy_fallback(), since the fd would only be used once. As a
> bonus, we would also not call g_file_query_info multiple times and not open
> source/dest files multiple times.
> 
> This ioctl() might actually fit in with the splice code as well.

I see; yeah, true.  It's a bit ugly to call G_IS_LOCAL_FILE inside gfile.c, but eh.

> What about g_local_file_delete()? This might sound silly, but I tend to bypass
> gio/glib wrappers only as a last resort. :)

A valid principle when writing apps, but here you're implementing GLib, which exists to wrap platform-specific API so others don't have to worry.
Comment 10 Colin Walters 2013-01-05 12:48:18 UTC
(In reply to comment #8)

> That's odd. I had simply copied gfile.h to create that header. I'll fix it.

Not sure where you got that from, but it wasn't GLib:

$ git describe
2.35.3-85-ge478b65
$ git grep 'red hat' | wc -l
0
$ git grep 'Red Hat' | wc -l
520
$

> I don't understand how that could happen. Won't that codepath lead to "goto
> out;", and exit the function with ret = FALSE?

Ah yes, you're right.

> I just thought it would aid in debugging in the future if this bit of extra
> information was passed on up the stack.

Ok.
Comment 11 Nirbheek Chauhan 2013-01-05 16:17:46 UTC
Created attachment 232827 [details] [review]
Btrfs reflink inside file_copy_fallback for GFile

This patch implements the ioctl inside file_copy_fallback()

(In reply to comment #9)
> I guess, but this seems like a problem best addressed by test coverage (an open
> question here, since GLib presently relies on "make distcheck" run on developer
> laptops which may or may not be running btrfs).
> 

I was wondering about that myself. Do you think it makes sense to add a test that checks for root, creates a loopback btrfs file, mounts it, and runs gio/tests/live-g-file inside it? We'd need a way to verify that reflinking actually happened, though.

> > This ioctl() might actually fit in with the splice code as well.
> 
> I see; yeah, true.  It's a bit ugly to call G_IS_LOCAL_FILE inside gfile.c, but
> eh.
> 

The attached patch assumes that G_IS_FILE_DESCRIPTOR_BASED is a subset of G_IS_LOCAL_FILE — is that a good assumption to make or do I need to call G_IS_LOCAL_FILE a well?

> > What about g_local_file_delete()? This might sound silly, but I tend to bypass
> > gio/glib wrappers only as a last resort. :)
> 
> A valid principle when writing apps, but here you're implementing GLib, which
> exists to wrap platform-specific API so others don't have to worry.

That's a good point. I'll keep that in mind for the future. :)

(In reply to comment #10)
> Not sure where you got that from, but it wasn't GLib:
> 
> $ git describe
> 2.35.3-85-ge478b65
> $ git grep 'red hat' | wc -l
> 0
> $ git grep 'Red Hat' | wc -l
> 520
> $
> 

I think I might've hit the wrong ViM keys and converted the whole block to lowercase while copying it! In any case that new header isn't needed any more.
Comment 12 Colin Walters 2013-01-05 18:52:34 UTC
Review of attachment 232827 [details] [review]:

This looks great to me.  Do you have a GNOME account?
Comment 13 Colin Walters 2013-01-05 18:55:00 UTC
(In reply to comment #11)
> 
> I was wondering about that myself. Do you think it makes sense to add a test
> that checks for root, creates a loopback btrfs file, mounts it, and runs
> gio/tests/live-g-file inside it? We'd need a way to verify that reflinking
> actually happened, though.

Pretty hacky, and as far as I know no one runs the GLib test suite as root anyways.

I do have ideas here but they involve radically redoing how we do tests, so it'll have to wait.

> The attached patch assumes that G_IS_FILE_DESCRIPTOR_BASED is a subset of
> G_IS_LOCAL_FILE — is that a good assumption to make or do I need to call
> G_IS_LOCAL_FILE a well?

Don't think it matters, because the ioctl will only work if both src and dest are btrfs, right?  So I think the current approach is fine.
Comment 14 Nirbheek Chauhan 2013-01-05 19:16:59 UTC
(In reply to comment #13)
> Pretty hacky, and as far as I know no one runs the GLib test suite as root
> anyways.
> 

> I do have ideas here but they involve radically redoing how we do tests, so
> it'll have to wait.
> 

All Gentoo users who run test-suites during installation run the GLib test suite as root. ;-)

But I know what you mean. I suppose a test suite for this can wait for now—I'd be happy to work on it when the time comes!

(FWIW, coreutils uses the above approach to test its reflink code; but that's a whole different beast.)

> > The attached patch assumes that G_IS_FILE_DESCRIPTOR_BASED is a subset of
> > G_IS_LOCAL_FILE — is that a good assumption to make or do I need to call
> > G_IS_LOCAL_FILE a well?
> 
> Don't think it matters, because the ioctl will only work if both src and dest
> are btrfs, right?  So I think the current approach is fine.

Yep, the ioctl will only work within the same filesystem (and only within the same subvolume for older kernels).

(In reply to comment #12)
> Review of attachment 232827 [details] [review]:
> 
> This looks great to me.  Do you have a GNOME account?

No, unfortunately I don't have one. Could you please commit it for me?
Comment 15 Colin Walters 2013-01-05 19:22:46 UTC
Comment on attachment 232827 [details] [review]
Btrfs reflink inside file_copy_fallback for GFile

I tweaked the commit message to include your original text, and added a link to this bug.
Comment 16 Colin Walters 2013-01-05 19:22:57 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Comment 17 Colin Walters 2013-01-25 15:52:23 UTC
Follow up crasher bug + patch: https://bugzilla.gnome.org/show_bug.cgi?id=692408