diff mbox

[v3,10/16] CIFS: Use multicredits for SMB 2.1/3 writes

Message ID CAKywueRfzhBFV7MV1SCLW6R+coxbmJFPNjc9inki+WXYkM-81g@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Shilovsky July 29, 2014, 7:56 a.m. UTC
2014-07-28 11:36 GMT+04:00 Pavel Shilovsky <pshilovsky@samba.org>:
> 2014-07-25 5:06 GMT+04:00 Shirish Pargaonkar <shirishpargaonkar@gmail.com>:
>> Looks correct.
>>
>> Reviewed-by: Shirish Pargaonkar <spargaonkar@suse.com>
>
> Thank you for reviewing the series!
>
>> Only  comment would be, wish there was a mnemonic/define for
>> a regular op  when calling add_credits_and_wake_if() for optype
>> such as CIFS_ECHO_OP or CIFS_OPBREAK_OP instead of 0.
>
> Yes, it can make sense but should be in another patch/series.
>
>> Oh and super nitpick...  s/reseted/reset/.
>
> Ok, will fix it in my smb2-dev branch on git.altlinux.org.
>
> --
> Best regards,
> Pavel Shilovsky.

Shirish,

I am going to add a check (as well as removing an unnecessary comment)
to write part:


If you are ok with the changes I will update 10/16 and 16/16 patches
and leave your Reviewed-by tags in my git tree. Thoughts?

Comments

Shirish Pargaonkar July 30, 2014, 2:06 a.m. UTC | #1
Pavel, that would be fine


On Tue, Jul 29, 2014 at 2:56 AM, Pavel Shilovsky <pshilovsky@samba.org> wrote:
> 2014-07-28 11:36 GMT+04:00 Pavel Shilovsky <pshilovsky@samba.org>:
>> 2014-07-25 5:06 GMT+04:00 Shirish Pargaonkar <shirishpargaonkar@gmail.com>:
>>> Looks correct.
>>>
>>> Reviewed-by: Shirish Pargaonkar <spargaonkar@suse.com>
>>
>> Thank you for reviewing the series!
>>
>>> Only  comment would be, wish there was a mnemonic/define for
>>> a regular op  when calling add_credits_and_wake_if() for optype
>>> such as CIFS_ECHO_OP or CIFS_OPBREAK_OP instead of 0.
>>
>> Yes, it can make sense but should be in another patch/series.
>>
>>> Oh and super nitpick...  s/reseted/reset/.
>>
>> Ok, will fix it in my smb2-dev branch on git.altlinux.org.
>>
>> --
>> Best regards,
>> Pavel Shilovsky.
>
> Shirish,
>
> I am going to add a check (as well as removing an unnecessary comment)
> to write part:
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 081529f..0dbd1de 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -230,6 +230,9 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon,
> struct smb_vol *volume_info)
>         wsize = volume_info->wsize ? volume_info->wsize : CIFS_DEFAULT_IOSIZE;
>         wsize = min_t(unsigned int, wsize, server->max_write);
>
> +       if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
> +               wsize = min_t(unsigned int, wsize, SMB2_MAX_BUFFER_SIZE);
> +
>         return wsize;
>  }
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 3acef4b..5a6842c 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -108,7 +108,6 @@ smb2_hdr_assemble(struct smb2_hdr *hdr, __le16
> smb2_cmd /* command */ ,
>         if (!tcon)
>                 goto out;
>
> -       /* BB FIXME when we do write > 64K add +1 for every 64K in req or rsp */
>         /* GLOBAL_CAP_LARGE_MTU will only be set if dialect > SMB2.02 */
>         /* See sections 2.2.4 and 3.2.4.1.5 of MS-SMB2 */
>         if ((tcon->ses) &&
>
> and the same check to read part:
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 0dbd1de..d0210a8 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -246,6 +246,9 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon,
> struct smb_vol *volume_info)
>         rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
>         rsize = min_t(unsigned int, rsize, server->max_read);
>
> +       if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
> +               rsize = min_t(unsigned int, rsize, SMB2_MAX_BUFFER_SIZE);
> +
>         return rsize;
>  }
>
> If you are ok with the changes I will update 10/16 and 16/16 patches
> and leave your Reviewed-by tags in my git tree. Thoughts?
>
> --
> Best regards,
> Pavel Shilovsky.
--
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
Pavel Shilovsky July 30, 2014, 7:41 a.m. UTC | #2
2014-07-30 6:06 GMT+04:00 Shirish Pargaonkar <shirishpargaonkar@gmail.com>:
> Pavel, that would be fine

Thank you - the branch is up to date now.

Steve, could you update your for-next branch with the recent version
of patches from my smb2-dev branch, please?
(http://git.altlinux.org/people/piastry/public/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev)

We haven't still decided if we need the patch "CIFS: Fix async reading
on reconnects" for stable. My opinion is yes since it fixes the real
problem that leads to lose of a data coherency. Also, the patch "CIFS:
Fix STATUS_CANNOT_DELETE error mapping for SMB2" is targeted for
stable as well.

Do we need to go ahead and send those two to v3.16 (and stable) before
it's released?
Steve French July 31, 2014, 4:23 a.m. UTC | #3
I tried to pull your git tree, but get an error

remote: aborting due to possible repository corruption on the remote side.
fatal: protocol error: bad pack header

will try again tomorrow to see if it is back up again.

On Wed, Jul 30, 2014 at 2:41 AM, Pavel Shilovsky <pshilovsky@samba.org> wrote:
> 2014-07-30 6:06 GMT+04:00 Shirish Pargaonkar <shirishpargaonkar@gmail.com>:
>> Pavel, that would be fine
>
> Thank you - the branch is up to date now.
>
> Steve, could you update your for-next branch with the recent version
> of patches from my smb2-dev branch, please?
> (http://git.altlinux.org/people/piastry/public/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev)
>
> We haven't still decided if we need the patch "CIFS: Fix async reading
> on reconnects" for stable. My opinion is yes since it fixes the real
> problem that leads to lose of a data coherency. Also, the patch "CIFS:
> Fix STATUS_CANNOT_DELETE error mapping for SMB2" is targeted for
> stable as well.
>
> Do we need to go ahead and send those two to v3.16 (and stable) before
> it's released?
>
> --
> Best regards,
> Pavel Shilovsky.
Pavel Shilovsky July 31, 2014, 6:38 a.m. UTC | #4
2014-07-31 8:23 GMT+04:00 Steve French <smfrench@gmail.com>:
> I tried to pull your git tree, but get an error
>
> remote: aborting due to possible repository corruption on the remote side.
> fatal: protocol error: bad pack header
>
> will try again tomorrow to see if it is back up again.

Strange enough, it is working from my side.

Anyway, I mirrored the branch at git.etersoft.ru:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev
Steve French Aug. 1, 2014, 5:58 a.m. UTC | #5
merged into cifs-2.6.git

On Thu, Jul 31, 2014 at 1:38 AM, Pavel Shilovsky <pshilovsky@samba.org> wrote:
> 2014-07-31 8:23 GMT+04:00 Steve French <smfrench@gmail.com>:
>> I tried to pull your git tree, but get an error
>>
>> remote: aborting due to possible repository corruption on the remote side.
>> fatal: protocol error: bad pack header
>>
>> will try again tomorrow to see if it is back up again.
>
> Strange enough, it is working from my side.
>
> Anyway, I mirrored the branch at git.etersoft.ru:
> http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev
>
> --
> Best regards,
> Pavel Shilovsky.
Pavel Shilovsky Aug. 1, 2014, 9:50 a.m. UTC | #6
2014-08-01 9:58 GMT+04:00 Steve French <smfrench@gmail.com>:
> merged into cifs-2.6.git

What's about the patch
http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=500cdea99fbbb3a487dad7ce0beba8d44b93a0fa
? It seems like a good candidate for stable@ series. Thoughts?
diff mbox

Patch

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 081529f..0dbd1de 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -230,6 +230,9 @@  smb2_negotiate_wsize(struct cifs_tcon *tcon,
struct smb_vol *volume_info)
        wsize = volume_info->wsize ? volume_info->wsize : CIFS_DEFAULT_IOSIZE;
        wsize = min_t(unsigned int, wsize, server->max_write);

+       if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
+               wsize = min_t(unsigned int, wsize, SMB2_MAX_BUFFER_SIZE);
+
        return wsize;
 }

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 3acef4b..5a6842c 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -108,7 +108,6 @@  smb2_hdr_assemble(struct smb2_hdr *hdr, __le16
smb2_cmd /* command */ ,
        if (!tcon)
                goto out;

-       /* BB FIXME when we do write > 64K add +1 for every 64K in req or rsp */
        /* GLOBAL_CAP_LARGE_MTU will only be set if dialect > SMB2.02 */
        /* See sections 2.2.4 and 3.2.4.1.5 of MS-SMB2 */
        if ((tcon->ses) &&

and the same check to read part:

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 0dbd1de..d0210a8 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -246,6 +246,9 @@  smb2_negotiate_rsize(struct cifs_tcon *tcon,
struct smb_vol *volume_info)
        rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
        rsize = min_t(unsigned int, rsize, server->max_read);

+       if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
+               rsize = min_t(unsigned int, rsize, SMB2_MAX_BUFFER_SIZE);
+
        return rsize;
 }