| Message ID | 1308658097-14805-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 21 Jun 2011 08:08:17 -0400 Jeff Layton <jlayton@redhat.com> wrote: > According to Hongwei Sun's blog posting here: > > http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx > > CAP_LARGE_WRITEX is ignored when signing is active. Also, the maximum > size for a write without CAP_LARGE_WRITEX should be the maxBuf that > the server sent in the NEGOTIATE request. > > Fix the wsize negotiation to take this into account. While we're at it, > alter the other wsize definitions to use sizeof(WRITE_REQ) to allow for > slightly larger amounts of data to potentially be written per request. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/connect.c | 24 +++++++++++++++--------- > 1 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index b15b5b0..f9a59b8 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -2754,21 +2754,21 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info, > > /* > * When the server supports very large writes via POSIX extensions, we can > - * allow up to 2^24 - PAGE_CACHE_SIZE. > + * allow up to 2^24 (16M) minus the size of a WRITE_AND_X header not including > + * the RFC1001 length. > * > * Note that this might make for "interesting" allocation problems during > * writeback however (as we have to allocate an array of pointers for the > * pages). A 16M write means ~32kb page array with PAGE_CACHE_SIZE == 4096. > */ > -#define CIFS_MAX_WSIZE ((1<<24) - PAGE_CACHE_SIZE) > +#define CIFS_MAX_WSIZE ((1<<24) - sizeof(WRITE_REQ) + 4) > > /* > * When the server doesn't allow large posix writes, default to a wsize of > - * 128k - PAGE_CACHE_SIZE -- one page less than the largest frame size > - * described in RFC1001. This allows space for the header without going over > - * that by default. > + * 128k minus the size of the WRITE_AND_X header. That allows for a write up > + * to the maximum size described by RFC1001. > */ > -#define CIFS_MAX_RFC1001_WSIZE (128 * 1024 - PAGE_CACHE_SIZE) > +#define CIFS_MAX_RFC1001_WSIZE (128 * 1024 - sizeof(WRITE_REQ) + 4) > > /* > * The default wsize is 1M. find_get_pages seems to return a maximum of 256 > @@ -2789,9 +2789,15 @@ cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info) > if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP)) > wsize = min_t(unsigned int, wsize, CIFS_MAX_RFC1001_WSIZE); > > - /* no CAP_LARGE_WRITE_X? Limit it to 16 bits */ > - if (!(server->capabilities & CAP_LARGE_WRITE_X)) > - wsize = min_t(unsigned int, wsize, USHRT_MAX); > + /* > + * no CAP_LARGE_WRITE_X or signing enabled? Limit to max buffer > + * offered by the server, minus the size of the WRITEX header, not > + * including the 4 byte RFC1001 length. > + */ > + if (!(server->capabilities & CAP_LARGE_WRITE_X) || > + (server->sec_mode & (SECMODE_SIGN_ENABLED|SECMODE_SIGN_REQUIRED))) > + wsize = min_t(unsigned int, wsize, > + server->maxBuf - sizeof(WRITE_REQ) + 4); > > /* hard limit of CIFS_MAX_WSIZE */ > wsize = min_t(unsigned int, wsize, CIFS_MAX_WSIZE); One problem with this patch. We probably shouldn't negotiate down the wsize when signing is enabled and unix extensions are in force (according to George Colley). Let me respin the patch to add that check as well.
Yep - makes sense - I will plan on merging the respun patch. Opinions about the rsize equivalent of this going in quickly as well? On Tue, Jun 21, 2011 at 10:33 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Tue, 21 Jun 2011 08:08:17 -0400 > Jeff Layton <jlayton@redhat.com> wrote: > >> According to Hongwei Sun's blog posting here: >> >> http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx >> >> CAP_LARGE_WRITEX is ignored when signing is active. Also, the maximum >> size for a write without CAP_LARGE_WRITEX should be the maxBuf that >> the server sent in the NEGOTIATE request. >> >> Fix the wsize negotiation to take this into account. While we're at it, >> alter the other wsize definitions to use sizeof(WRITE_REQ) to allow for >> slightly larger amounts of data to potentially be written per request. >> >> Signed-off-by: Jeff Layton <jlayton@redhat.com> >> --- >> fs/cifs/connect.c | 24 +++++++++++++++--------- >> 1 files changed, 15 insertions(+), 9 deletions(-) >> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> index b15b5b0..f9a59b8 100644 >> --- a/fs/cifs/connect.c >> +++ b/fs/cifs/connect.c >> @@ -2754,21 +2754,21 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info, >> >> /* >> * When the server supports very large writes via POSIX extensions, we can >> - * allow up to 2^24 - PAGE_CACHE_SIZE. >> + * allow up to 2^24 (16M) minus the size of a WRITE_AND_X header not including >> + * the RFC1001 length. >> * >> * Note that this might make for "interesting" allocation problems during >> * writeback however (as we have to allocate an array of pointers for the >> * pages). A 16M write means ~32kb page array with PAGE_CACHE_SIZE == 4096. >> */ >> -#define CIFS_MAX_WSIZE ((1<<24) - PAGE_CACHE_SIZE) >> +#define CIFS_MAX_WSIZE ((1<<24) - sizeof(WRITE_REQ) + 4) >> >> /* >> * When the server doesn't allow large posix writes, default to a wsize of >> - * 128k - PAGE_CACHE_SIZE -- one page less than the largest frame size >> - * described in RFC1001. This allows space for the header without going over >> - * that by default. >> + * 128k minus the size of the WRITE_AND_X header. That allows for a write up >> + * to the maximum size described by RFC1001. >> */ >> -#define CIFS_MAX_RFC1001_WSIZE (128 * 1024 - PAGE_CACHE_SIZE) >> +#define CIFS_MAX_RFC1001_WSIZE (128 * 1024 - sizeof(WRITE_REQ) + 4) >> >> /* >> * The default wsize is 1M. find_get_pages seems to return a maximum of 256 >> @@ -2789,9 +2789,15 @@ cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info) >> if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP)) >> wsize = min_t(unsigned int, wsize, CIFS_MAX_RFC1001_WSIZE); >> >> - /* no CAP_LARGE_WRITE_X? Limit it to 16 bits */ >> - if (!(server->capabilities & CAP_LARGE_WRITE_X)) >> - wsize = min_t(unsigned int, wsize, USHRT_MAX); >> + /* >> + * no CAP_LARGE_WRITE_X or signing enabled? Limit to max buffer >> + * offered by the server, minus the size of the WRITEX header, not >> + * including the 4 byte RFC1001 length. >> + */ >> + if (!(server->capabilities & CAP_LARGE_WRITE_X) || >> + (server->sec_mode & (SECMODE_SIGN_ENABLED|SECMODE_SIGN_REQUIRED))) >> + wsize = min_t(unsigned int, wsize, >> + server->maxBuf - sizeof(WRITE_REQ) + 4); >> >> /* hard limit of CIFS_MAX_WSIZE */ >> wsize = min_t(unsigned int, wsize, CIFS_MAX_WSIZE); > > One problem with this patch. We probably shouldn't negotiate down the > wsize when signing is enabled and unix extensions are in force > (according to George Colley). Let me respin the patch to add that check > as well. > > -- > Jeff Layton <jlayton@redhat.com> >
On Tue, 21 Jun 2011 11:24:12 -0500 Steve French <smfrench@gmail.com> wrote: > Yep - makes sense - I will plan on merging the respun patch. > > Opinions about the rsize equivalent of this going in quickly as well? > According to George, reading doesn't have this issue and Hongwei's blog is wrong in this respect. That makes some sense since for a read we're sending a very small request to the server, and we specify how much we want to receive. Eventually, cleaning up the rsize negotiation would be good, but I don't think there's any urgency for it.
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index b15b5b0..f9a59b8 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2754,21 +2754,21 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info, /* * When the server supports very large writes via POSIX extensions, we can - * allow up to 2^24 - PAGE_CACHE_SIZE. + * allow up to 2^24 (16M) minus the size of a WRITE_AND_X header not including + * the RFC1001 length. * * Note that this might make for "interesting" allocation problems during * writeback however (as we have to allocate an array of pointers for the * pages). A 16M write means ~32kb page array with PAGE_CACHE_SIZE == 4096. */ -#define CIFS_MAX_WSIZE ((1<<24) - PAGE_CACHE_SIZE) +#define CIFS_MAX_WSIZE ((1<<24) - sizeof(WRITE_REQ) + 4) /* * When the server doesn't allow large posix writes, default to a wsize of - * 128k - PAGE_CACHE_SIZE -- one page less than the largest frame size - * described in RFC1001. This allows space for the header without going over - * that by default. + * 128k minus the size of the WRITE_AND_X header. That allows for a write up + * to the maximum size described by RFC1001. */ -#define CIFS_MAX_RFC1001_WSIZE (128 * 1024 - PAGE_CACHE_SIZE) +#define CIFS_MAX_RFC1001_WSIZE (128 * 1024 - sizeof(WRITE_REQ) + 4) /* * The default wsize is 1M. find_get_pages seems to return a maximum of 256 @@ -2789,9 +2789,15 @@ cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info) if (!tcon->unix_ext || !(unix_cap & CIFS_UNIX_LARGE_WRITE_CAP)) wsize = min_t(unsigned int, wsize, CIFS_MAX_RFC1001_WSIZE); - /* no CAP_LARGE_WRITE_X? Limit it to 16 bits */ - if (!(server->capabilities & CAP_LARGE_WRITE_X)) - wsize = min_t(unsigned int, wsize, USHRT_MAX); + /* + * no CAP_LARGE_WRITE_X or signing enabled? Limit to max buffer + * offered by the server, minus the size of the WRITEX header, not + * including the 4 byte RFC1001 length. + */ + if (!(server->capabilities & CAP_LARGE_WRITE_X) || + (server->sec_mode & (SECMODE_SIGN_ENABLED|SECMODE_SIGN_REQUIRED))) + wsize = min_t(unsigned int, wsize, + server->maxBuf - sizeof(WRITE_REQ) + 4); /* hard limit of CIFS_MAX_WSIZE */ wsize = min_t(unsigned int, wsize, CIFS_MAX_WSIZE);
According to Hongwei Sun's blog posting here: http://blogs.msdn.com/b/openspecification/archive/2009/04/10/smb-maximum-transmit-buffer-size-and-performance-tuning.aspx CAP_LARGE_WRITEX is ignored when signing is active. Also, the maximum size for a write without CAP_LARGE_WRITEX should be the maxBuf that the server sent in the NEGOTIATE request. Fix the wsize negotiation to take this into account. While we're at it, alter the other wsize definitions to use sizeof(WRITE_REQ) to allow for slightly larger amounts of data to potentially be written per request. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/connect.c | 24 +++++++++++++++--------- 1 files changed, 15 insertions(+), 9 deletions(-)