[PATCH 0/2] btrfs-progs: receive: address setattr failures

[PATCH 0/2] btrfs-progs: receive: address setattr failures

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

I was writing a test case for fstests with fsstress and send v2 streams
and it turns out that receive will always fail if we have a fileattr
command in the stream. It fails for two different reasons, described in
the changelog.

We actually had a report today of a receiver failing on a send stream v2
because of the reason addressed in the first patch. However fixing that
alone, will still cause the receiver to fail, just for a different reason
(patch 2).

For now I'll hold the test case for fstests since it fails very frequently
(almost always actually).

Filipe Manana (2):
  btrfs-progs: receive: fix parsing of attributes field from the fileattr command
  btrfs-progs: receive: work around failure of fileattr commands

 cmds/receive.c       | 44 ++++++++++++++++++++++++++------------------
 common/send-stream.c |  2 +-
 2 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.35.1

[PATCH 1/2] btrfs-progs: receive: fix parsing of attributes field from the fileattr command

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We're trying to get a U32 for the attributes, but the kernel sends a U64
(which is correct as we store attributes in a u64 flags field of the
inode). This makes anyone trying to receive a v2 send stream to fail with:

    ERROR: invalid size for attribute, expected = 4, got = 8

We actually recently got such a report of someone using send stream v2 and
getting such failure. See the Link tag below.

Link: https://lore.kernel.org/linux-btrfs/6cb11fa5-c60d-e65b-0295-301a694e66ad@inbox.ru/
Fixes: 7a6fb356dc65 ("btrfs-progs: receive: process setflags ioctl commands")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 common/send-stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/send-stream.c b/common/send-stream.c
index 72a25729..e9f565e6 100644
--- a/common/send-stream.c
+++ b/common/send-stream.c
@@ -570,7 +570,7 @@ static int read_and_process_cmd(struct btrfs_send_stream *sctx)
 		break;
 	case BTRFS_SEND_C_FILEATTR:
 		TLV_GET_STRING(sctx, BTRFS_SEND_A_PATH, &path);
-		TLV_GET_U32(sctx, BTRFS_SEND_A_FILEATTR, &fileattr);
+		TLV_GET_U64(sctx, BTRFS_SEND_A_FILEATTR, &fileattr);
 		ret = sctx->ops->fileattr(path, fileattr, sctx->user);
 		break;
 	}
-- 
2.35.1

[PATCH 2/2] btrfs-progs: receive: work around failure of fileattr commands

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Currently fileattr commands, introduced in the send stream v2, always
fail, since we have commented the FS_IOC_SETFLAGS ioctl() call and set
'ret' to -EOPNOTSUPP, which is then overwritten to -errno, which may
have a random value since it was not initialized before. This results
in a failure like this:

   ERROR: fileattr: set file attributes on p0/f1 failed: Invalid argument

The error reason may be something else, since errno is undefined at
this point.

Unfortunatelly we don't have a way yet to apply attributes, since the
attributes value we get from the kernel is what we store in flags field
of the inode item. This means that for example we can not just call
FS_IOC_SETFLAGS with the values we got, since they need to be converted
from BTRFS_INODE_* flags to FS_* flags

Besides that we'll have to reorder how we apply certain attributes like
FS_NOCOW_FL for example, which must happen always on an empty file and
right now we run write commands before attempting to change attributes,
as that's the order the kernel sends the operations.

So for now comment all the code, so that anyone using the v2 stream will
not have a receive failure but will get a behaviour like the v1 stream:
file attributes are ignored. This will have to be fixed later, but right
now people can't use a send stream v2 for the purpose of getting better
performance by avoid decompressing extents at the source and compression
of the data at the destination.

Link: https://lore.kernel.org/linux-btrfs/6cb11fa5-c60d-e65b-0295-301a694e66ad@inbox.ru/
Fixes: 8356c423e619 ("btrfs-progs: receive: implement FILEATTR command")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 cmds/receive.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/cmds/receive.c b/cmds/receive.c
index af3138d5..6f475544 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -1304,25 +1304,33 @@ static int process_fallocate(const char *path, int mode, u64 offset, u64 len,
 
 static int process_fileattr(const char *path, u64 attr, void *user)
 {
-	int ret;
-	struct btrfs_receive *rctx = user;
-	char full_path[PATH_MAX];
+	/*
+	 * Not yet supported, ignored for now, just like in send stream v1.
+	 * The content of 'attr' matches the flags in the btrfs inode item,
+	 * we can't apply them directly with FS_IOC_SETFLAGS, as we need to
+	 * convert them from BTRFS_INODE_* flags to FS_* flags. Plus some
+	 * flags are special and must be applied in a special way.
+	 * The commented code below therefore does not work.
+	 */
 
-	ret = path_cat_out(full_path, rctx->full_subvol_path, path);
-	if (ret < 0) {
-		error("fileattr: path invalid: %s", path);
-		return ret;
-	}
-	ret = open_inode_for_write(rctx, full_path);
-	if (ret < 0)
-		return ret;
-	ret = -EOPNOTSUPP;
-	/* ret = ioctl(rctx->write_fd, FS_IOC_SETFLAGS, &flags); */
-	if (ret < 0) {
-		ret = -errno;
-		error("fileattr: set file attributes on %s failed: %m", path);
-		return ret;
-	}
+	/* int ret; */
+	/* struct btrfs_receive *rctx = user; */
+	/* char full_path[PATH_MAX]; */
+
+	/* ret = path_cat_out(full_path, rctx->full_subvol_path, path); */
+	/* if (ret < 0) { */
+	/* 	error("fileattr: path invalid: %s", path); */
+	/* 	return ret; */
+	/* } */
+	/* ret = open_inode_for_write(rctx, full_path); */
+	/* if (ret < 0) */
+	/* 	return ret; */
+	/* ret = ioctl(rctx->write_fd, FS_IOC_SETFLAGS, &attr); */
+	/* if (ret < 0) { */
+	/* 	ret = -errno; */
+	/* 	error("fileattr: set file attributes on %s failed: %m", path); */
+	/* 	return ret; */
+	/* } */
 	return 0;
 }
 
-- 
2.35.1

Re: [PATCH 0/2] btrfs-progs: receive: address setattr failures

[David Sterba]

On Thu, Nov 03, 2022 at 04:53:25PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> I was writing a test case for fstests with fsstress and send v2 streams
> and it turns out that receive will always fail if we have a fileattr
> command in the stream. It fails for two different reasons, described in
> the changelog.
> 
> We actually had a report today of a receiver failing on a send stream v2
> because of the reason addressed in the first patch. However fixing that
> alone, will still cause the receiver to fail, just for a different reason
> (patch 2).
> 
> For now I'll hold the test case for fstests since it fails very frequently
> (almost always actually).
> 
> Filipe Manana (2):
>   btrfs-progs: receive: fix parsing of attributes field from the fileattr command
>   btrfs-progs: receive: work around failure of fileattr commands

Added to devel, thanks.