| Message ID | 20140802142449.15f4cfc2c2821e90e04e175e@skynet.be (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;