Re: [PATCH 3/7] cifs: don't declare smb_vol info on the stack

Re: [PATCH 3/7] cifs: don't declare smb_vol info on the stack

[Steve French]

I have thought about this a few times, but always hesitated because I
was hoping that eventually cifs mount option parsing could use new
parsing functions (which would also shrink the "parse_mount_options"
routine which we call during cifs mount), and would cause us to
rewrite this.  IIRC nfs was changed a few years ago to use the new
mount mechanism (match_table and tokens).

On Sun, Nov 30, 2008 at 12:40 PM, Jeff Layton <jlayton@redhat.com> wrote:
> struct smb_vol is fairly large, it's probably best to kzalloc it...
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/connect.c |   91 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 49 insertions(+), 42 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 291ccd6..b08c6a3 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2186,27 +2186,30 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  {
>        int rc = 0;
>        int xid;
> -       struct smb_vol volume_info;
> +       struct smb_vol *volume_info;
>        struct cifsSesInfo *pSesInfo = NULL;
>        struct cifsTconInfo *tcon = NULL;
>        struct TCP_Server_Info *srvTcp = NULL;
>
>        xid = GetXid();
>
> -/* cFYI(1, ("Entering cifs_mount. Xid: %d with: %s", xid, mount_data)); */
> +       volume_info = kzalloc(sizeof(struct smb_vol), GFP_KERNEL);
> +       if (!volume_info) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
>
> -       memset(&volume_info, 0, sizeof(struct smb_vol));
> -       if (cifs_parse_mount_options(mount_data, devname, &volume_info)) {
> +       if (cifs_parse_mount_options(mount_data, devname, volume_info)) {
>                rc = -EINVAL;
>                goto out;
>        }
>
> -       if (volume_info.nullauth) {
> +       if (volume_info->nullauth) {
>                cFYI(1, ("null user"));
> -               volume_info.username = "";
> -       } else if (volume_info.username) {
> +               volume_info->username = "";
> +       } else if (volume_info->username) {
>                /* BB fixme parse for domain name here */
> -               cFYI(1, ("Username: %s", volume_info.username));
> +               cFYI(1, ("Username: %s", volume_info->username));
>        } else {
>                cifserror("No username specified");
>        /* In userspace mount helper we can get user name from alternate
> @@ -2217,27 +2220,27 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>
>
>        /* this is needed for ASCII cp to Unicode converts */
> -       if (volume_info.iocharset == NULL) {
> +       if (volume_info->iocharset == NULL) {
>                cifs_sb->local_nls = load_nls_default();
>        /* load_nls_default can not return null */
>        } else {
> -               cifs_sb->local_nls = load_nls(volume_info.iocharset);
> +               cifs_sb->local_nls = load_nls(volume_info->iocharset);
>                if (cifs_sb->local_nls == NULL) {
>                        cERROR(1, ("CIFS mount error: iocharset %s not found",
> -                                volume_info.iocharset));
> +                                volume_info->iocharset));
>                        rc = -ELIBACC;
>                        goto out;
>                }
>        }
>
>        /* get a reference to a tcp session */
> -       srvTcp = cifs_get_tcp_session(&volume_info);
> +       srvTcp = cifs_get_tcp_session(volume_info);
>        if (IS_ERR(srvTcp)) {
>                rc = PTR_ERR(srvTcp);
>                goto out;
>        }
>
> -       pSesInfo = cifs_find_smb_ses(srvTcp, volume_info.username);
> +       pSesInfo = cifs_find_smb_ses(srvTcp, volume_info->username);
>        if (pSesInfo) {
>                cFYI(1, ("Existing smb sess found (status=%d)",
>                        pSesInfo->status));
> @@ -2275,24 +2278,24 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                list_add(&pSesInfo->smb_ses_list, &srvTcp->smb_ses_list);
>                write_unlock(&cifs_tcp_ses_lock);
>
> -               /* volume_info.password freed at unmount */
> -               if (volume_info.password) {
> -                       pSesInfo->password = volume_info.password;
> +               /* volume_info->password freed at unmount */
> +               if (volume_info->password) {
> +                       pSesInfo->password = volume_info->password;
>                        /* set to NULL to prevent freeing on exit */
> -                       volume_info.password = NULL;
> +                       volume_info->password = NULL;
>                }
> -               if (volume_info.username)
> -                       strncpy(pSesInfo->userName, volume_info.username,
> +               if (volume_info->username)
> +                       strncpy(pSesInfo->userName, volume_info->username,
>                                MAX_USERNAME_SIZE);
> -               if (volume_info.domainname) {
> -                       int len = strlen(volume_info.domainname);
> +               if (volume_info->domainname) {
> +                       int len = strlen(volume_info->domainname);
>                        pSesInfo->domainName = kmalloc(len + 1, GFP_KERNEL);
>                        if (pSesInfo->domainName)
>                                strcpy(pSesInfo->domainName,
> -                                       volume_info.domainname);
> +                                       volume_info->domainname);
>                }
> -               pSesInfo->linux_uid = volume_info.linux_uid;
> -               pSesInfo->overrideSecFlg = volume_info.secFlg;
> +               pSesInfo->linux_uid = volume_info->linux_uid;
> +               pSesInfo->overrideSecFlg = volume_info->secFlg;
>                down(&pSesInfo->sesSem);
>
>                /* BB FIXME need to pass vol->secFlgs BB */
> @@ -2303,14 +2306,14 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>
>        /* search for existing tcon to this server share */
>        if (!rc) {
> -               setup_cifs_sb(&volume_info, cifs_sb);
> +               setup_cifs_sb(volume_info, cifs_sb);
>
> -               tcon = cifs_find_tcon(pSesInfo, volume_info.UNC);
> +               tcon = cifs_find_tcon(pSesInfo, volume_info->UNC);
>                if (tcon) {
>                        cFYI(1, ("Found match on UNC path"));
>                        /* existing tcon already has a reference */
>                        cifs_put_smb_ses(pSesInfo);
> -                       if (tcon->seal != volume_info.seal)
> +                       if (tcon->seal != volume_info->seal)
>                                cERROR(1, ("transport encryption setting "
>                                           "conflicts with existing tid"));
>                } else {
> @@ -2322,8 +2325,8 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                        tcon->ses = pSesInfo;
>
>                        /* check for null share name ie connect to dfs root */
> -                       if ((strchr(volume_info.UNC + 3, '\\') == NULL)
> -                           && (strchr(volume_info.UNC + 3, '/') == NULL)) {
> +                       if ((strchr(volume_info->UNC + 3, '\\') == NULL)
> +                           && (strchr(volume_info->UNC + 3, '/') == NULL)) {
>                                /* rc = connect_to_dfs_path(...) */
>                                cFYI(1, ("DFS root not supported"));
>                                rc = -ENODEV;
> @@ -2332,10 +2335,10 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                                /* BB Do we need to wrap sesSem around
>                                 * this TCon call and Unix SetFS as
>                                 * we do on SessSetup and reconnect? */
> -                               rc = CIFSTCon(xid, pSesInfo, volume_info.UNC,
> +                               rc = CIFSTCon(xid, pSesInfo, volume_info->UNC,
>                                              tcon, cifs_sb->local_nls);
>                                cFYI(1, ("CIFS Tcon rc = %d", rc));
> -                               if (volume_info.nodfs) {
> +                               if (volume_info->nodfs) {
>                                        tcon->Flags &= ~SMB_SHARE_IS_IN_DFS;
>                                        cFYI(1, ("DFS disabled (%d)",
>                                                tcon->Flags));
> @@ -2343,7 +2346,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                        }
>                        if (rc)
>                                goto mount_fail_check;
> -                       tcon->seal = volume_info.seal;
> +                       tcon->seal = volume_info->seal;
>                        write_lock(&cifs_tcp_ses_lock);
>                        list_add(&tcon->tcon_list, &pSesInfo->tcon_list);
>                        write_unlock(&cifs_tcp_ses_lock);
> @@ -2353,9 +2356,9 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                   to a share so for resources mounted more than once
>                   to the same server share the last value passed in
>                   for the retry flag is used */
> -               tcon->retry = volume_info.retry;
> -               tcon->nocase = volume_info.nocase;
> -               tcon->local_lease = volume_info.local_lease;
> +               tcon->retry = volume_info->retry;
> +               tcon->nocase = volume_info->nocase;
> +               tcon->local_lease = volume_info->local_lease;
>        }
>        if (pSesInfo) {
>                if (pSesInfo->capabilities & CAP_LARGE_FILES) {
> @@ -2392,7 +2395,7 @@ mount_fail_check:
>        if (tcon->ses->capabilities & CAP_UNIX)
>                /* reset of caps checks mount to see if unix extensions
>                   disabled for just this mount */
> -               reset_cifs_unix_caps(xid, tcon, sb, &volume_info);
> +               reset_cifs_unix_caps(xid, tcon, sb, volume_info);
>        else
>                tcon->unix_ext = 0; /* server does not support them */
>
> @@ -2411,18 +2414,22 @@ mount_fail_check:
>                cifs_sb->rsize = min(cifs_sb->rsize,
>                               (tcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE));
>
> -       /* volume_info.password is freed above when existing session found
> +       /* volume_info->password is freed above when existing session found
>        (in which case it is not needed anymore) but when new sesion is created
>        the password ptr is put in the new session structure (in which case the
>        password will be freed at unmount time) */
>  out:
>        /* zero out password before freeing */
> -       if (volume_info.password != NULL) {
> -               memset(volume_info.password, 0, strlen(volume_info.password));
> -               kfree(volume_info.password);
> +       if (volume_info) {
> +               if (volume_info->password != NULL) {
> +                       memset(volume_info->password, 0,
> +                               strlen(volume_info->password));
> +                       kfree(volume_info->password);
> +               }
> +               kfree(volume_info->UNC);
> +               kfree(volume_info->prepath);
> +               kfree(volume_info);
>        }
> -       kfree(volume_info.UNC);
> -       kfree(volume_info.prepath);
>        FreeXid(xid);
>        return rc;
>  }
> --
> 1.5.5.1
>
>



-- 
Thanks,

Steve

Re: [PATCH 3/7] cifs: don't declare smb_vol info on the stack

[Jeff Layton]

On Sun, 30 Nov 2008 14:49:04 -0600
"Steve French" <smfrench@gmail.com> wrote:

> I have thought about this a few times, but always hesitated because I
> was hoping that eventually cifs mount option parsing could use new
> parsing functions (which would also shrink the "parse_mount_options"
> routine which we call during cifs mount), and would cause us to
> rewrite this.  IIRC nfs was changed a few years ago to use the new
> mount mechanism (match_table and tokens).
> 

I think we'll still have a smb_vol struct or something like it,
regardless of how we end up doing the parsing. The results of the
parsing have to be recorded somewhere. There's a similar struct for NFS
as well (nfs_parsed_mount_data). We definitely should switch to the
standard parser, but I don't think this patch will make much difference
in how that is implemented.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/7] cifs: clean up socket creation, connection and sending (try #2)

[Steve French]

After review of Jeff's 2 recent patch series, and discussion with Jeff
on IRC (as a result Jeff respun and made minor modifications to a
couple), I have just merged 10 of his 14 patches into cifs-2.6.git.
See below:

3 min ago 	Jeff Layton 	cifs: make ipv6_connect take a TCP_Server_Info
3 min ago 	Jeff Layton 	cifs: make ipv4_connect take a TCP_Server_Info
14 min ago 	Jeff Layton 	cifs: don't declare smb_vol info on the stack
15 min ago 	Jeff Layton 	cifs: move allocation of new TCP_Server_Info
into separ ...
7 min ago 	Jeff Layton 	cifs: account for IPv6 in ses->serverName and clean ...
21 min ago 	Jeff Layton 	cifs: make dnotify thread experimental code
5 hours ago 	Jeff Layton 	cifs: convert tcpSem to a mutex
5 hours ago 	Jeff Layton 	cifs: take module reference when starting cifsd
6 hours ago 	Jeff Layton 	cifs: display addr and prefixpath options in /proc ...
6 hours ago 	Jeff Layton 	cifs: remove unused SMB session pointer from
struct ...

On Sun, Nov 30, 2008 at 12:40 PM, Jeff Layton <jlayton@redhat.com> wrote:
> This patchset is intended as a cleanup of the code that creates and
> connects sockets. It's also intended to unify the smb_send and smb_send2
> functions, and simplify the sending code in the blocking I/O case. There
> is a lot of duplicate code in those functions, and this should make it
> easier to ensure that we fix bugs with it properly. This should also fix
> the case where we get a partial send in smb_send. Previously, we did not
> force a reconnect in that case.
>
> This is the second post of this patchset. The main differences from the
> original set are:
>
> 1) fix some symbol naming style issues that HCH pointed out
>
> 2) move the address handling into cifs_get_tcp_server and out of
>   cifs_mount altogether (also pointed out by HCH)
>
> 3) the addition of a patch to reduce stack consumption in cifs_mount by
>   dynamically allocating the volume_info
>
> This patchset is also intended for 2.6.29 and depends on the 5 patch set
> of cleanups that I sent last week.
>
> I've only given this set cursory testing. If it looks OK, I'll plan to
> do some more with it. The last set was more heavily tested, and showed
> no functional or performance regressions as best I could tell. Since the
> differences between that set and this one are fairly superficial, I
> don't expect much difference in performance here.
>
> Jeff Layton (7):
>  cifs: convert tcpSem to a mutex
>  cifs: move allocation of new TCP_Server_Info into separate function
>  cifs: don't declare smb_vol info on the stack
>  cifs: make ipv4_connect take a TCP_Server_Info arg
>  cifs: make ipv6_connect take a TCP_Server_Info arg
>  cifs: turn smb_send into a wrapper around smb_sendv
>  cifs: don't retry on blocking sends
>
>  fs/cifs/cifsglob.h  |    2 +-
>  fs/cifs/cifsproto.h |    4 +-
>  fs/cifs/connect.c   |  609 ++++++++++++++++++++++++++-------------------------
>  fs/cifs/transport.c |  147 ++++---------
>  4 files changed, 359 insertions(+), 403 deletions(-)
>
>



-- 
Thanks,

Steve

Re: [PATCH 0/7] cifs: clean up socket creation, connection and sending (try #2)

[Jeff Layton]

Thanks Steve, 

> 21 min ago 	Jeff Layton 	cifs: make dnotify thread experimental code

I think that this patch ^^^ may cause processes to hang indefinitely if the
server goes unresponsive. I think the dnotify thread is the only thing that
prevents this since it wakes things up every 15s. It would be good to get
the wait_for_response patch in soon...

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>