| Message ID | CAH2r5muLwHY_e8QJP5N4veyqem4mQ9rOJC87nOeHYVyvFmWLsw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 16 Nov 2013 01:14:19 -0600 Steve French <smfrench@gmail.com> wrote: > fs/cifs/smb2ops.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++------- > fs/cifs/smb2pdu.c | 2 +- > 2 files changed, 79 insertions(+), 12 deletions(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 11dde4b..7a21447 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -532,7 +532,10 @@ smb2_clone_range(const unsigned int xid, > int rc; > unsigned int ret_data_len; > struct copychunk_ioctl *pcchunk; > - char *retbuf = NULL; > + struct copychunk_ioctl_rsp *retbuf = NULL; > + struct cifs_tcon *tcon; > + int chunks_copied = 0; > + bool chunk_sizes_updated = false; > > pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL); > > @@ -552,22 +555,86 @@ smb2_clone_range(const unsigned int xid, The SMB2_request_res_key() error path above here should be fixed to free pcchunk. > /* For now array only one chunk long, will make more flexible later */ > pcchunk->ChunkCount = __constant_cpu_to_le32(1); > pcchunk->Reserved = 0; > - pcchunk->SourceOffset = cpu_to_le64(src_off); > - pcchunk->TargetOffset = cpu_to_le64(dest_off); > - pcchunk->Length = cpu_to_le32(len); > pcchunk->Reserved2 = 0; > > + tcon = tlink_tcon(trgtfile->tlink); > + > + while (len > 0) { > + pcchunk->SourceOffset = cpu_to_le64(src_off); > + pcchunk->TargetOffset = cpu_to_le64(dest_off); > + pcchunk->Length = > + cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); > + > /* Request that server copy to target from src file identified by key */ Comment should be indented here too. > - rc = SMB2_ioctl(xid, tlink_tcon(trgtfile->tlink), > - trgtfile->fid.persistent_fid, > + rc = SMB2_ioctl(xid, tcon, trgtfile->fid.persistent_fid, > trgtfile->fid.volatile_fid, FSCTL_SRV_COPYCHUNK_WRITE, > true /* is_fsctl */, (char *)pcchunk, > - sizeof(struct copychunk_ioctl), &retbuf, &ret_data_len); > - > - /* BB need to special case rc = EINVAL to alter chunk size */ > - > - cifs_dbg(FYI, "rc %d data length out %d\n", rc, ret_data_len); > + sizeof(struct copychunk_ioctl), (char **)&retbuf, > + &ret_data_len); > + if (rc == 0) { > + if (ret_data_len != sizeof(struct copychunk_ioctl_rsp)) > + goto cchunk_out; > + if (retbuf->TotalBytesWritten == 0) { > + cifs_dbg(FYI, "no bytes copied\n"); > + goto cchunk_out; > + } These two error cases should set rc non-zero, it may not be the last copy-chunk for this clone. ... > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1214,7 +1214,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, > rc = SendReceive2(xid, ses, iov, num_iovecs, &resp_buftype, 0); > rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base; > > - if (rc != 0) { > + if ((rc != 0) && (rc != -EINVAL)) { This should probably be FSCTL_SRV_COPYCHUNK[_WRITE] specific. See MS-SMB2 3.3.4.4 Sending an Error Response When the server is responding with a failure to any command sent by the client, the response message MUST be constructed as described here. An error code other than one of the following indicates a failure: ... STATUS_INVALID_PARAMETER in an FSCTL_SRV_COPYCHUNK or FSCTL_SRV_COPYCHUNK_WRITE response, when returning an SRV_COPYCHUNK_RESPONSE as described in section 3.3.5.15.6.2. > if (tcon) > cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); > goto ioctl_exit; Cheers, David -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
sending updated patch to include your feedback On Sat, Nov 16, 2013 at 12:50 PM, David Disseldorp <ddiss@suse.de> wrote: > On Sat, 16 Nov 2013 01:14:19 -0600 > Steve French <smfrench@gmail.com> wrote: > >> fs/cifs/smb2ops.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++------- >> fs/cifs/smb2pdu.c | 2 +- >> 2 files changed, 79 insertions(+), 12 deletions(-) >> >> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c >> index 11dde4b..7a21447 100644 >> --- a/fs/cifs/smb2ops.c >> +++ b/fs/cifs/smb2ops.c >> @@ -532,7 +532,10 @@ smb2_clone_range(const unsigned int xid, >> int rc; >> unsigned int ret_data_len; >> struct copychunk_ioctl *pcchunk; >> - char *retbuf = NULL; >> + struct copychunk_ioctl_rsp *retbuf = NULL; >> + struct cifs_tcon *tcon; >> + int chunks_copied = 0; >> + bool chunk_sizes_updated = false; >> >> pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL); >> >> @@ -552,22 +555,86 @@ smb2_clone_range(const unsigned int xid, > > The SMB2_request_res_key() error path above here should be fixed to free > pcchunk. > >> /* For now array only one chunk long, will make more flexible later */ >> pcchunk->ChunkCount = __constant_cpu_to_le32(1); >> pcchunk->Reserved = 0; >> - pcchunk->SourceOffset = cpu_to_le64(src_off); >> - pcchunk->TargetOffset = cpu_to_le64(dest_off); >> - pcchunk->Length = cpu_to_le32(len); >> pcchunk->Reserved2 = 0; >> >> + tcon = tlink_tcon(trgtfile->tlink); >> + >> + while (len > 0) { >> + pcchunk->SourceOffset = cpu_to_le64(src_off); >> + pcchunk->TargetOffset = cpu_to_le64(dest_off); >> + pcchunk->Length = >> + cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); >> + >> /* Request that server copy to target from src file identified by key */ > > Comment should be indented here too. > >> - rc = SMB2_ioctl(xid, tlink_tcon(trgtfile->tlink), >> - trgtfile->fid.persistent_fid, >> + rc = SMB2_ioctl(xid, tcon, trgtfile->fid.persistent_fid, >> trgtfile->fid.volatile_fid, FSCTL_SRV_COPYCHUNK_WRITE, >> true /* is_fsctl */, (char *)pcchunk, >> - sizeof(struct copychunk_ioctl), &retbuf, &ret_data_len); >> - >> - /* BB need to special case rc = EINVAL to alter chunk size */ >> - >> - cifs_dbg(FYI, "rc %d data length out %d\n", rc, ret_data_len); >> + sizeof(struct copychunk_ioctl), (char **)&retbuf, >> + &ret_data_len); >> + if (rc == 0) { >> + if (ret_data_len != sizeof(struct copychunk_ioctl_rsp)) >> + goto cchunk_out; >> + if (retbuf->TotalBytesWritten == 0) { >> + cifs_dbg(FYI, "no bytes copied\n"); >> + goto cchunk_out; >> + } > > These two error cases should set rc non-zero, it may not be the last > copy-chunk for this clone. > ... > >> --- a/fs/cifs/smb2pdu.c >> +++ b/fs/cifs/smb2pdu.c >> @@ -1214,7 +1214,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, >> rc = SendReceive2(xid, ses, iov, num_iovecs, &resp_buftype, 0); >> rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base; >> >> - if (rc != 0) { >> + if ((rc != 0) && (rc != -EINVAL)) { > > This should probably be FSCTL_SRV_COPYCHUNK[_WRITE] specific. See MS-SMB2 > 3.3.4.4 Sending an Error Response > When the server is responding with a failure to any command sent by the > client, the response message MUST be constructed as described here. An > error code other than one of the following indicates a failure: > ... > > STATUS_INVALID_PARAMETER in an FSCTL_SRV_COPYCHUNK or > FSCTL_SRV_COPYCHUNK_WRITE response, when returning an > SRV_COPYCHUNK_RESPONSE as described in section 3.3.5.15.6.2. > > >> if (tcon) >> cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); >> goto ioctl_exit; > > Cheers, David
From 6474d967fc8228fff7579748f7219f6a8e443f30 Mon Sep 17 00:00:00 2001 From: Steve French <smfrench@gmail.com> Date: Sat, 16 Nov 2013 01:06:00 -0600 Subject: [PATCH] CIFS: SMB2/SMB3 Copy offload support (refcopy) finish up This patch extends the ability of copychunk (refcopy) to handle servers with smaller than usual maximum chunk sizes and also to handle files bigger than the maximum chunk sizes Signed-off-by: Steve French <smfrench@gmail.com> --- fs/cifs/smb2ops.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++------- fs/cifs/smb2pdu.c | 2 +- 2 files changed, 79 insertions(+), 12 deletions(-) diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 11dde4b..7a21447 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -532,7 +532,10 @@ smb2_clone_range(const unsigned int xid, int rc; unsigned int ret_data_len; struct copychunk_ioctl *pcchunk; - char *retbuf = NULL; + struct copychunk_ioctl_rsp *retbuf = NULL; + struct cifs_tcon *tcon; + int chunks_copied = 0; + bool chunk_sizes_updated = false; pcchunk = kmalloc(sizeof(struct copychunk_ioctl), GFP_KERNEL); @@ -552,22 +555,86 @@ smb2_clone_range(const unsigned int xid, /* For now array only one chunk long, will make more flexible later */ pcchunk->ChunkCount = __constant_cpu_to_le32(1); pcchunk->Reserved = 0; - pcchunk->SourceOffset = cpu_to_le64(src_off); - pcchunk->TargetOffset = cpu_to_le64(dest_off); - pcchunk->Length = cpu_to_le32(len); pcchunk->Reserved2 = 0; + tcon = tlink_tcon(trgtfile->tlink); + + while (len > 0) { + pcchunk->SourceOffset = cpu_to_le64(src_off); + pcchunk->TargetOffset = cpu_to_le64(dest_off); + pcchunk->Length = + cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); + /* Request that server copy to target from src file identified by key */ - rc = SMB2_ioctl(xid, tlink_tcon(trgtfile->tlink), - trgtfile->fid.persistent_fid, + rc = SMB2_ioctl(xid, tcon, trgtfile->fid.persistent_fid, trgtfile->fid.volatile_fid, FSCTL_SRV_COPYCHUNK_WRITE, true /* is_fsctl */, (char *)pcchunk, - sizeof(struct copychunk_ioctl), &retbuf, &ret_data_len); - - /* BB need to special case rc = EINVAL to alter chunk size */ - - cifs_dbg(FYI, "rc %d data length out %d\n", rc, ret_data_len); + sizeof(struct copychunk_ioctl), (char **)&retbuf, + &ret_data_len); + if (rc == 0) { + if (ret_data_len != sizeof(struct copychunk_ioctl_rsp)) + goto cchunk_out; + if (retbuf->TotalBytesWritten == 0) { + cifs_dbg(FYI, "no bytes copied\n"); + goto cchunk_out; + } + /* + * Check if server claimed to write more than we asked + */ + if (le32_to_cpu(retbuf->TotalBytesWritten) > + le32_to_cpu(pcchunk->Length)) { + cifs_dbg(VFS, "invalid copy chunk response\n"); + rc = -EIO; + goto cchunk_out; + } + if (le32_to_cpu(retbuf->ChunksWritten) != 1) { + cifs_dbg(VFS, "invalid num chunks written\n"); + rc = -EIO; + goto cchunk_out; + } + chunks_copied++; + + src_off += le32_to_cpu(retbuf->TotalBytesWritten); + dest_off += le32_to_cpu(retbuf->TotalBytesWritten); + len -= le32_to_cpu(retbuf->TotalBytesWritten); + + cifs_dbg(FYI, "Chunks %d PartialChunk %d Total %d\n", + le32_to_cpu(retbuf->ChunksWritten), + le32_to_cpu(retbuf->ChunkBytesWritten), + le32_to_cpu(retbuf->TotalBytesWritten)); + } else if (rc == -EINVAL) { + if (ret_data_len != sizeof(struct copychunk_ioctl_rsp)) + goto cchunk_out; + + cifs_dbg(FYI, "MaxChunks %d BytesChunk %d MaxCopy %d\n", + le32_to_cpu(retbuf->ChunksWritten), + le32_to_cpu(retbuf->ChunkBytesWritten), + le32_to_cpu(retbuf->TotalBytesWritten)); + + /* + * Check if this is the first request using these sizes, + * (ie check if copy succeed once with original sizes + * and check if the server gave us different sizes after + * we already updated max sizes on previous request). + * if not then why is the server returning an error now + */ + if ((chunks_copied != 0) || chunk_sizes_updated) + goto cchunk_out; + + /* Check that server is not asking us to grow size */ + if (le32_to_cpu(retbuf->ChunkBytesWritten) < + tcon->max_bytes_chunk) + tcon->max_bytes_chunk = + le32_to_cpu(retbuf->ChunkBytesWritten); + else + goto cchunk_out; /* server gave us bogus size */ + + /* No need to change MaxChunks since already set to 1 */ + chunk_sizes_updated = true; + } + } +cchunk_out: kfree(pcchunk); return rc; } diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index d65270c..c5915dc 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1214,7 +1214,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, rc = SendReceive2(xid, ses, iov, num_iovecs, &resp_buftype, 0); rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base; - if (rc != 0) { + if ((rc != 0) && (rc != -EINVAL)) { if (tcon) cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); goto ioctl_exit; -- 1.8.3.1