[PATCH 1/1] Btrfs: fix sparse warning

[PATCH 1/1] Btrfs: fix sparse warning

[Fabian Frederick]

Fix the following sparse warning:
fs/btrfs/send.c:518:51: warning: incorrect type in argument 2 (different address spaces)
fs/btrfs/send.c:518:51:    expected char const [noderef] <asn:1>*<noident>
fs/btrfs/send.c:518:51:    got char *

We can safely use (const char __user *) with set_fs(KERNEL_DS)

__force added to avoid sparse-all warning:
fs/btrfs/send.c:518:40: warning: cast adds address space to expression (<asn:1>)

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---

This is untested.

 fs/btrfs/send.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 6528aa6..b67e12e 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
 	set_fs(KERNEL_DS);
 
 	while (pos < len) {
-		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
+		ret = vfs_write(filp, (__force const char __user *)buf + pos,
+				len - pos, off);
 		/* TODO handle that correctly */
 		/*if (ret == -ERESTARTSYS) {
 			continue;
-- 
1.8.4.5

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Zach Brown]

On Tue, Jul 15, 2014 at 09:17:17PM +0200, Fabian Frederick wrote:
> Fix the following sparse warning:
> fs/btrfs/send.c:518:51: warning: incorrect type in argument 2 (different address spaces)
> fs/btrfs/send.c:518:51:    expected char const [noderef] <asn:1>*<noident>
> fs/btrfs/send.c:518:51:    got char *
> 
> We can safely use (const char __user *) with set_fs(KERNEL_DS)

Yeah, that cast is correct.

Reviewed-by: Zach Brown <zab@zabbo.net>

> @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)

Though this probably wants to be rewritten in terms of kernel_write().
That'd give an opportunity to get rid of the sctx->send_off and have it
use f_pos in the filp.

- z

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Fabian Frederick]

> On 16 July 2014 at 19:20 Zach Brown <zab@zabbo.net> wrote:
>
>
> On Tue, Jul 15, 2014 at 09:17:17PM +0200, Fabian Frederick wrote:
> > Fix the following sparse warning:
> > fs/btrfs/send.c:518:51: warning: incorrect type in argument 2 (different
> > address spaces)
> > fs/btrfs/send.c:518:51:    expected char const [noderef] *
> > fs/btrfs/send.c:518:51:    got char *
> >
> > We can safely use (const char __user *) with set_fs(KERNEL_DS)
>
> Yeah, that cast is correct.
>
> Reviewed-by: Zach Brown <zab@zabbo.net>
>
> > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf,
> > u32 len, loff_t *off)
>
> Though this probably wants to be rewritten in terms of kernel_write().
> That'd give an opportunity to get rid of the sctx->send_off and have it
> use f_pos in the filp.

Do you mean directly call kernel_write from send_cmd/send_header ?
I guess that loop around vfs_write in write_buf is there for something ...

Fabian

>
> - z

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Zach Brown]

> > > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf,
> > > u32 len, loff_t *off)
> >
> > Though this probably wants to be rewritten in terms of kernel_write().
> > That'd give an opportunity to get rid of the sctx->send_off and have it
> > use f_pos in the filp.
> 
> Do you mean directly call kernel_write from send_cmd/send_header ?
> I guess that loop around vfs_write in write_buf is there for something ...

write_buf() could still exist to iterate over the buffer in the case of
partial writes but it doesn't need to muck around with set_fs() and
forcing casts.

- z

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Fabian Frederick]

On Thu, 17 Jul 2014 12:01:52 -0700
Zach Brown <zab@zabbo.net> wrote:

> > > > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf,
> > > > u32 len, loff_t *off)
> > >
> > > Though this probably wants to be rewritten in terms of kernel_write().
> > > That'd give an opportunity to get rid of the sctx->send_off and have it
> > > use f_pos in the filp.
> > 
> > Do you mean directly call kernel_write from send_cmd/send_header ?
> > I guess that loop around vfs_write in write_buf is there for something ...
> 
> write_buf() could still exist to iterate over the buffer in the case of
> partial writes but it doesn't need to muck around with set_fs() and
> forcing casts.
> 
> - z

Hello Zach,

	Here's an untested patch which

	-removes send_off and set_fs
	-calls kernel_write with ppos 0

but I don't know if something like adding filp->f_pos = pos
at the end of write_buf would be enough to replace send_off ...

Regards,
Fabian 
	

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 6528aa6..a144b4e 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -83,7 +83,6 @@ struct clone_root {
 
 struct send_ctx {
 	struct file *send_filp;
-	loff_t send_off;
 	char *send_buf;
 	u32 send_size;
 	u32 send_max_size;
@@ -505,35 +504,28 @@ static struct btrfs_path *alloc_path_for_send(void)
 	return path;
 }
 
-static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
+static int write_buf(struct file *filp, const void *buf, u32 len)
 {
 	int ret;
 	mm_segment_t old_fs;
 	u32 pos = 0;
 
-	old_fs = get_fs();
-	set_fs(KERNEL_DS);
-
 	while (pos < len) {
-		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
+		ret = kernel_write(filp, (char *)buf + pos, len - pos, 0);
 		/* TODO handle that correctly */
 		/*if (ret == -ERESTARTSYS) {
 			continue;
 		}*/
 		if (ret < 0)
-			goto out;
+			return ret;
 		if (ret == 0) {
 			ret = -EIO;
-			goto out;
+			return ret;
 		}
 		pos += ret;
 	}
 
-	ret = 0;
-
-out:
-	set_fs(old_fs);
-	return ret;
+	return 0;
 }
 
 static int tlv_put(struct send_ctx *sctx, u16 attr, const void *data, int len)
@@ -639,8 +631,7 @@ static int send_header(struct send_ctx *sctx)
 	strcpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
 	hdr.version = cpu_to_le32(BTRFS_SEND_STREAM_VERSION);
 
-	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
-					&sctx->send_off);
+	return write_buf(sctx->send_filp, &hdr, sizeof(hdr));
 }
 
 /*
@@ -675,8 +666,7 @@ static int send_cmd(struct send_ctx *sctx)
 	crc = btrfs_crc32c(0, (unsigned char *)sctx->send_buf, sctx->send_size);
 	hdr->crc = cpu_to_le32(crc);
 
-	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
-					&sctx->send_off);
+	ret = write_buf(sctx->send_filp, sctx->send_buf);
 
 	sctx->total_send_size += sctx->send_size;
 	sctx->cmd_send_size[le16_to_cpu(hdr->cmd)] += sctx->send_size;

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Zach Brown]

On Sat, Aug 02, 2014 at 02:24:49PM +0200, Fabian Frederick wrote:
> On Thu, 17 Jul 2014 12:01:52 -0700
> Zach Brown <zab@zabbo.net> wrote:
> 
> > > > > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf,
> > > > > u32 len, loff_t *off)
> > > >
> > > > Though this probably wants to be rewritten in terms of kernel_write().
> > > > That'd give an opportunity to get rid of the sctx->send_off and have it
> > > > use f_pos in the filp.
> > > 
> > > Do you mean directly call kernel_write from send_cmd/send_header ?
> > > I guess that loop around vfs_write in write_buf is there for something ...
> > 
> > write_buf() could still exist to iterate over the buffer in the case of
> > partial writes but it doesn't need to muck around with set_fs() and
> > forcing casts.
> > 
> > - z
> 
> Hello Zach,
> 
> 	Here's an untested patch which

Try testing it.  It's easy with virtualization and xfstests.

You'll find that sending to a file fails because each individual file
write call that makes up a send starts at offset 0 -- at the start of
the file.

Getting this right means getting the semantics around updating the send
descriptors f_pos right.  It requires having a bit of a think about send
semantics and f_pos update locking.

- z

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Fabian Frederick]

> On 04 August 2014 at 20:31 Zach Brown <zab@zabbo.net> wrote:
>
>
> On Sat, Aug 02, 2014 at 02:24:49PM +0200, Fabian Frederick wrote:
> > On Thu, 17 Jul 2014 12:01:52 -0700
> > Zach Brown <zab@zabbo.net> wrote:
> >
> > > > > > @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const
> > > > > > void *buf,
> > > > > > u32 len, loff_t *off)
> > > > >
> > > > > Though this probably wants to be rewritten in terms of kernel_write().
> > > > > That'd give an opportunity to get rid of the sctx->send_off and have
> > > > > it
> > > > > use f_pos in the filp.
> > > >
> > > > Do you mean directly call kernel_write from send_cmd/send_header ?
> > > > I guess that loop around vfs_write in write_buf is there for something
> > > > ...
> > >
> > > write_buf() could still exist to iterate over the buffer in the case of
> > > partial writes but it doesn't need to muck around with set_fs() and
> > > forcing casts.
> > >
> > > - z
> >
> > Hello Zach,
> >
> >     Here's an untested patch which
>
> Try testing it.  It's easy with virtualization and xfstests.
>
> You'll find that sending to a file fails because each individual file
> write call that makes up a send starts at offset 0 -- at the start of
> the file.
>
> Getting this right means getting the semantics around updating the send
> descriptors f_pos right.  It requires having a bit of a think about send
> semantics and f_pos update locking.

Thanks for those informations Zach,

I've tried btrfs test scripts related to ioctl in xfstests (tests/btrfs/025,
035, 052, 055)
but was not able to trigger that problem. Do I have to create another script,
use some generic one
or maybe use big test/scratch devices ?

Regards,
Fabian
>
> - z

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Zach Brown]

> > > Hello Zach,
> > >
> > >     Here's an untested patch which
> >
> > Try testing it.  It's easy with virtualization and xfstests.
> >
> > You'll find that sending to a file fails because each individual file
> > write call that makes up a send starts at offset 0 -- at the start of
> > the file.
> >
> > Getting this right means getting the semantics around updating the send
> > descriptors f_pos right.  It requires having a bit of a think about send
> > semantics and f_pos update locking.
> 
> Thanks for those informations Zach,
> 
> I've tried btrfs test scripts related to ioctl in xfstests (tests/btrfs/025,
> 035, 052, 055)
> but was not able to trigger that problem. Do I have to create another script,
> use some generic one
> or maybe use big test/scratch devices ?

No idea, sorry.  Maybe your patch is fine and I'm a dummy.  Maybe you
didn't test the kernel you thought you were testing.  Maybe the test
doesn't test what you changed.  You'll have to do some investigating to
find out.

- z

Re: [PATCH 1/1] Btrfs: fix sparse warning

[Fabian Frederick]

> On 05 August 2014 at 23:32 Zach Brown <zab@zabbo.net> wrote:
>
>
> > > > Hello Zach,
> > > >
> > > >     Here's an untested patch which
> > >
> > > Try testing it.  It's easy with virtualization and xfstests.
> > >
> > > You'll find that sending to a file fails because each individual file
> > > write call that makes up a send starts at offset 0 -- at the start of
> > > the file.
> > >
> > > Getting this right means getting the semantics around updating the send
> > > descriptors f_pos right.  It requires having a bit of a think about send
> > > semantics and f_pos update locking.
> >
> > Thanks for those informations Zach,
> >
> > I've tried btrfs test scripts related to ioctl in xfstests (tests/btrfs/025,
> > 035, 052, 055)
> > but was not able to trigger that problem. Do I have to create another
> > script,
> > use some generic one
> > or maybe use big test/scratch devices ?
>
> No idea, sorry.  Maybe your patch is fine and I'm a dummy.  Maybe you
> didn't test the kernel you thought you were testing.  Maybe the test
> doesn't test what you changed.  You'll have to do some investigating to
> find out.

After checking both kernel and btrfs-progs, it seems file offset is only being
passed through TLV(see send.c/ send_write/
TLV_PUT_U64(BTRFS_SEND_A_FILE_OFFSET). Tell me if I'm wrong but when we do
'btrfs send backup  | btrfs receive backupvolume',
btrfs-progs calls btrfs_ioctl_send emitting several BTRFS_SEND_C_WRITE with
relevant BTRFS_SEND_A_FILE_OFFSET (growing by 48kb). At the other side of | ,
'btrfs receive' executes read_and_process_cmd which does TLV_GET before
s->ops->write. All btrfs interactions being done with | maybe we could simply
replace vfs_write stuff as you suggested and remove sctx->send_off ?

Regards,
Fabian 

>
> - z