Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAEUGjKiLPQP9wp0AgLUvHgKBOe9We2a-RQaZ7cd7CvhnarwWiw@mail.gmail.com>
In-Reply-To: <CAEUGjKiLPQP9wp0AgLUvHgKBOe9We2a-RQaZ7cd7CvhnarwWiw@mail.gmail.com>
From:   Pavel Shilovsky <piastryyy@gmail.com>
Date:   Fri, 10 Apr 2020 11:25:01 -0700
Message-ID: <CAKywueT0Q9WkANNsg8cEDwGZSMaaE5c4LHuEeMhVDzJAzycroQ@mail.gmail.com>
Subject: Re: [PATCH] cifs: improve read performance for page size 64KB &
 cache=strict & vers=2.1+
To:     Jones Syue <jonessyue@qnap.com>
Cc:     linux-cifs <linux-cifs@vger.kernel.org>,
        Samba Technical <samba-technical@lists.samba.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=D0=BF=D1=82, 10 =D0=B0=D0=BF=D1=80. 2020 =D0=B3. =D0=B2 09:50, Jones Syue =
via samba-technical
<samba-technical@lists.samba.org>:
>
> Hello list,
>
> please help review whether the attached patch is correct, thank you.
>
> Found a read performance issue when linux kernel page size is 64KB.
> If linux kernel page size is 64KB and mount options cache=3Dstrict &
> vers=3D2.1+,
> it does not support cifs_readpages(). Instead, it is using cifs_readpage(=
)
> and
> cifs_read() with maximum read IO size 16KB, which is much slower than rea=
d
> IO
> size 1MB when negotiated SMB 2.1+. Since modern SMB server supported SMB
> 2.1+
> and Max Read Size can reach more than 64KB (for example 1MB ~ 8MB), this
> patch
> do one more check on max_read to determine whether server support
> readpages()
> and improve read performance for page size 64KB & cache=3Dstrict & vers=
=3D2.1+.
>
> The client is a linux box with linux kernel 4.2.8,
> page size 64KB (CONFIG_ARM64_64K_PAGES=3Dy),
> cpu arm 1.7GHz, and use mount.cifs as smb client.
> The server is another linux box with linux kernel 4.2.8,
> share a file '10G.img' with size 10GB,
> and use samba-4.7.12 as smb server.
>
> The client mount a share from the server with different
> cache options: cache=3Dstrict and cache=3Dnone,
> mount -tcifs //<server_ip>/Public /cache_strict
> -overs=3D3.0,cache=3Dstrict,username=3D<xxx>,password=3D<yyy>
> mount -tcifs //<server_ip>/Public /cache_none
> -overs=3D3.0,cache=3Dnone,username=3D<xxx>,password=3D<yyy>
>
> The client download a 10GbE file from the server accross 1GbE network,
> dd if=3D/cache_strict/10G.img of=3D/dev/null bs=3D1M count=3D10240
> dd if=3D/cache_none/10G.img of=3D/dev/null bs=3D1M count=3D10240
>
> Found that cache=3Dstrict (without patch) is slower read throughput and
> smaller
> read IO size than cache=3Dnone.
> cache=3Dstrict (without patch): read throughput 40MB/s, read IO size is 1=
6KB
> cache=3Dstrict (with patch): read throughput 113MB/s, read IO size is 1MB
> cache=3Dnone: read throughput 109MB/s, read IO size is 1MB
>
> Looks like if page size is 64KB,
> cifs_set_ops() would use cifs_addr_ops_smallbuf instead of cifs_addr_ops,
>
> /* check if server can support readpages */
> if (cifs_sb_master_tcon(cifs_sb)->ses->server->maxBuf <
> PAGE_SIZE + MAX_CIFS_HDR_SIZE)
> inode->i_data.a_ops =3D &cifs_addr_ops_smallbuf;
> else
> inode->i_data.a_ops =3D &cifs_addr_ops;
>
> maxBuf is came from 2 places, SMB2_negotiate() and CIFSSMBNegotiate(),
> (SMB2_MAX_BUFFER_SIZE is 64KB)
> SMB2_negotiate():
> /* set it to the maximum buffer size value we can send with 1 credit */
> server->maxBuf =3D min_t(unsigned int, le32_to_cpu(rsp->MaxTransactSize),
>       SMB2_MAX_BUFFER_SIZE);
> CIFSSMBNegotiate():
> server->maxBuf =3D le32_to_cpu(pSMBr->MaxBufferSize);
>
> Page size 64KB and cache=3Dstrict lead to read_pages() use cifs_readpage(=
)
> instead
> of cifs_readpages(), and then cifs_read() using maximum read IO size 16KB=
,
> which is much slower than maximum read IO size 1MB.
> (CIFSMaxBufSize is 16KB by default)
>
> /* FIXME: set up handlers for larger reads and/or convert to async */
> rsize =3D min_t(unsigned int, cifs_sb->rsize, CIFSMaxBufSize);
>

Hi Jones,

Thanks for the patch!

It will work although it is probably a little bit cleaner to
initialize server->max_read to server->maxBuf for SMB1 and use the
server->max_read in the readpages condition check instead.

@Others, thoughts?

--
Best regards,
Pavel Shilovsky