diff mbox

[v4,4/4] CIFS: Migrate to shared superblock model

Message ID 1306389721-28550-4-git-send-email-piastry@etersoft.ru (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Shilovsky May 26, 2011, 6:02 a.m. UTC
Add cifs_match_super to use in sget to share superblock between mounts
that have the same //server/sharename, credentials and mount options.
It helps us to improve performance on work with future SMB2.1 leases.

Reviewed-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru>
---
 fs/cifs/cifsfs.c   |   19 ++++++++++-
 fs/cifs/cifsglob.h |   19 +++++++++++
 fs/cifs/connect.c  |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 1 deletions(-)

Comments

Jeff Layton May 26, 2011, 2:09 p.m. UTC | #1
On Thu, 26 May 2011 10:02:01 +0400
Pavel Shilovsky <piastry@etersoft.ru> wrote:

> Add cifs_match_super to use in sget to share superblock between mounts
> that have the same //server/sharename, credentials and mount options.
> It helps us to improve performance on work with future SMB2.1 leases.
> 
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru>
> ---
>  fs/cifs/cifsfs.c   |   19 ++++++++++-
>  fs/cifs/cifsglob.h |   19 +++++++++++
>  fs/cifs/connect.c  |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 46fdd55..360fe2e 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -630,6 +630,7 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	struct super_block *sb;
>  	struct cifs_sb_info *cifs_sb;
>  	struct smb_vol *volume_info;
> +	struct cifs_mnt_data mnt_data;
>  	struct dentry *root;
>  
>  	cFYI(1, "Devname: %s flags: %d ", dev_name, flags);
> @@ -646,12 +647,21 @@ cifs_do_mount(struct file_system_type *fs_type,
>  
>  	cifs_setup_cifs_sb(volume_info, cifs_sb);
>  
> -	sb = sget(fs_type, NULL, set_anon_super, NULL);
> +	mnt_data.vol = volume_info;
> +	mnt_data.cifs_sb = cifs_sb;
> +	mnt_data.flags = flags;
> +
> +	sb = sget(fs_type, cifs_match_super, set_anon_super, &mnt_data);
>  	if (IS_ERR(sb)) {
>  		root = ERR_CAST(sb);
>  		goto out_cifs_sb;
>  	}
>  
> +	if (sb->s_fs_info) {
> +		cFYI(1, "Use existing superblock");
> +		goto out_shared;
> +	}
> +
>  	/*
>  	 * Copy mount params for use in submounts. Better to do
>  	 * the copy here and deal with the error before cleanup gets
> @@ -680,9 +690,16 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	root = cifs_get_root(volume_info, sb);
>  	if (root == NULL)
>  		goto out_super;
> +
>  	cFYI(1, "dentry root is: %p", root);
>  	goto out;
>  
> +out_shared:
> +	root = cifs_get_root(volume_info, sb);
> +	if (root)
> +		cFYI(1, "dentry root is: %p", root);
> +	goto out;
> +
>  out_super:
>  	kfree(cifs_sb->mountdata);
>  	deactivate_locked_super(sb);
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f57387a..7f90488 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -210,6 +210,25 @@ struct smb_vol {
>  	struct nls_table *local_nls;
>  };
>  
> +#define CIFS_MOUNT_MASK (CIFS_MOUNT_NO_PERM | CIFS_MOUNT_SET_UID | \
> +			 CIFS_MOUNT_SERVER_INUM | CIFS_MOUNT_DIRECT_IO | \
> +			 CIFS_MOUNT_NO_XATTR | CIFS_MOUNT_MAP_SPECIAL_CHR | \
> +			 CIFS_MOUNT_UNX_EMUL | CIFS_MOUNT_NO_BRL | \
> +			 CIFS_MOUNT_CIFS_ACL | CIFS_MOUNT_OVERR_UID | \
> +			 CIFS_MOUNT_OVERR_GID | CIFS_MOUNT_DYNPERM | \
> +			 CIFS_MOUNT_NOPOSIXBRL | CIFS_MOUNT_NOSSYNC | \
> +			 CIFS_MOUNT_FSCACHE | CIFS_MOUNT_MF_SYMLINKS | \
> +			 CIFS_MOUNT_MULTIUSER | CIFS_MOUNT_STRICT_IO)
> +
> +#define CIFS_MS_MASK (MS_RDONLY | MS_MANDLOCK | MS_NOEXEC | MS_NOSUID | \
> +		      MS_NODEV | MS_SYNCHRONOUS)
> +
> +struct cifs_mnt_data {
> +	struct cifs_sb_info *cifs_sb;
> +	struct smb_vol *vol;
> +	int flags;
> +};
> +
>  struct TCP_Server_Info {
>  	struct list_head tcp_ses_list;
>  	struct list_head smb_ses_list;
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4f34519..3172baa 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2146,6 +2146,96 @@ cifs_put_tlink(struct tcon_link *tlink)
>  	return;
>  }
>  
> +static inline struct tcon_link *
> +cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb);
> +
> +static int
> +compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
> +{
> +	struct cifs_sb_info *old = CIFS_SB(sb);
> +	struct cifs_sb_info *new = mnt_data->cifs_sb;
> +
> +	if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
> +		return 0;
> +
> +	if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
> +	    (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
> +		return 0;
> +
> +	if (old->rsize != new->rsize)
> +		return 0;
> +
> +	if (new->wsize && new->wsize > old->wsize)
> +		return 0;
> +

Currently, this will never be checked as the wsize in the new sb isn't
set yet and will always be 0. I think the right fix is to have
cifs_setup_cifs_sb() set the wsize in the new sb unconditionally to
whatever it was set to in the smb_vol. If we end up using the new sb
then cifs_negotiate_wsize will later negotiate it downward if needed.

> +	if (old->mnt_uid != new->mnt_uid || old->mnt_gid != new->mnt_gid)
> +		return 0;
> +
> +	if (old->mnt_file_mode != new->mnt_file_mode ||
> +	    old->mnt_dir_mode != new->mnt_dir_mode)
> +		return 0;
> +
> +	if (strcmp(old->local_nls->charset, new->local_nls->charset))
> +		return 0;
> +
> +	if (old->actimeo != new->actimeo)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +int
> +cifs_match_super(struct super_block *sb, void *data)
> +{
> +	struct cifs_mnt_data *mnt_data = (struct cifs_mnt_data *)data;
> +	struct smb_vol *volume_info;
> +	struct cifs_sb_info *cifs_sb;
> +	struct TCP_Server_Info *tcp_srv;
> +	struct cifsSesInfo *ses;
> +	struct cifsTconInfo *tcon;
> +	struct tcon_link *tlink;
> +	struct sockaddr_storage addr;
> +	int rc = 0;
> +
> +	memset(&addr, 0, sizeof(struct sockaddr_storage));
> +
> +	spin_lock(&cifs_tcp_ses_lock);
> +	cifs_sb = CIFS_SB(sb);
> +	tlink = cifs_get_tlink(cifs_sb_master_tlink(cifs_sb));
> +	if (IS_ERR(tlink)) {
> +		spin_unlock(&cifs_tcp_ses_lock);
> +		return rc;
> +	}
> +	tcon = tlink_tcon(tlink);
> +	ses = tcon->ses;
> +	tcp_srv = ses->server;
> +
> +	volume_info = mnt_data->vol;
> +
> +	if (!volume_info->UNCip || !volume_info->UNC)
> +		goto out;
> +
> +	rc = cifs_fill_sockaddr((struct sockaddr *)&addr,
> +				volume_info->UNCip,
> +				strlen(volume_info->UNCip),
> +				volume_info->port);
> +	if (!rc)
> +		goto out;
> +
> +	if (!match_server(tcp_srv, (struct sockaddr *)&addr, volume_info) ||
> +	    !match_session(ses, volume_info) ||
> +	    !match_tcon(tcon, volume_info->UNC)) {
> +		rc = 0;
> +		goto out;
> +	}
> +
> +	rc = compare_mount_options(sb, mnt_data);
> +out:
> +	cifs_put_tlink(tlink);
> +	spin_unlock(&cifs_tcp_ses_lock);
> +	return rc;
> +}
> +
>  int
>  get_dfs_path(int xid, struct cifsSesInfo *pSesInfo, const char *old_path,
>  	     const struct nls_table *nls_codepage, unsigned int *pnum_referrals,
Jeff Layton May 26, 2011, 3:20 p.m. UTC | #2
On Thu, 26 May 2011 10:02:01 +0400
Pavel Shilovsky <piastry@etersoft.ru> wrote:

> Add cifs_match_super to use in sget to share superblock between mounts
> that have the same //server/sharename, credentials and mount options.
> It helps us to improve performance on work with future SMB2.1 leases.
> 
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru>
> ---
>  fs/cifs/cifsfs.c   |   19 ++++++++++-
>  fs/cifs/cifsglob.h |   19 +++++++++++
>  fs/cifs/connect.c  |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 46fdd55..360fe2e 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -630,6 +630,7 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	struct super_block *sb;
>  	struct cifs_sb_info *cifs_sb;
>  	struct smb_vol *volume_info;
> +	struct cifs_mnt_data mnt_data;
>  	struct dentry *root;
>  
>  	cFYI(1, "Devname: %s flags: %d ", dev_name, flags);
> @@ -646,12 +647,21 @@ cifs_do_mount(struct file_system_type *fs_type,
>  
>  	cifs_setup_cifs_sb(volume_info, cifs_sb);
>  
> -	sb = sget(fs_type, NULL, set_anon_super, NULL);
> +	mnt_data.vol = volume_info;
> +	mnt_data.cifs_sb = cifs_sb;
> +	mnt_data.flags = flags;
> +
> +	sb = sget(fs_type, cifs_match_super, set_anon_super, &mnt_data);
>  	if (IS_ERR(sb)) {
>  		root = ERR_CAST(sb);
>  		goto out_cifs_sb;
>  	}
>  
> +	if (sb->s_fs_info) {
> +		cFYI(1, "Use existing superblock");
> +		goto out_shared;
> +	}
> +
>  	/*
>  	 * Copy mount params for use in submounts. Better to do
>  	 * the copy here and deal with the error before cleanup gets
> @@ -680,9 +690,16 @@ cifs_do_mount(struct file_system_type *fs_type,
>  	root = cifs_get_root(volume_info, sb);
>  	if (root == NULL)
>  		goto out_super;
> +
>  	cFYI(1, "dentry root is: %p", root);
>  	goto out;
>  
> +out_shared:
> +	root = cifs_get_root(volume_info, sb);
> +	if (root)
> +		cFYI(1, "dentry root is: %p", root);
> +	goto out;
> +
>  out_super:
>  	kfree(cifs_sb->mountdata);
>  	deactivate_locked_super(sb);
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f57387a..7f90488 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -210,6 +210,25 @@ struct smb_vol {
>  	struct nls_table *local_nls;
>  };
>  
> +#define CIFS_MOUNT_MASK (CIFS_MOUNT_NO_PERM | CIFS_MOUNT_SET_UID | \
> +			 CIFS_MOUNT_SERVER_INUM | CIFS_MOUNT_DIRECT_IO | \
> +			 CIFS_MOUNT_NO_XATTR | CIFS_MOUNT_MAP_SPECIAL_CHR | \
> +			 CIFS_MOUNT_UNX_EMUL | CIFS_MOUNT_NO_BRL | \
> +			 CIFS_MOUNT_CIFS_ACL | CIFS_MOUNT_OVERR_UID | \
> +			 CIFS_MOUNT_OVERR_GID | CIFS_MOUNT_DYNPERM | \
> +			 CIFS_MOUNT_NOPOSIXBRL | CIFS_MOUNT_NOSSYNC | \
> +			 CIFS_MOUNT_FSCACHE | CIFS_MOUNT_MF_SYMLINKS | \
> +			 CIFS_MOUNT_MULTIUSER | CIFS_MOUNT_STRICT_IO)
> +
> +#define CIFS_MS_MASK (MS_RDONLY | MS_MANDLOCK | MS_NOEXEC | MS_NOSUID | \
> +		      MS_NODEV | MS_SYNCHRONOUS)
> +
> +struct cifs_mnt_data {
> +	struct cifs_sb_info *cifs_sb;
> +	struct smb_vol *vol;
> +	int flags;
> +};
> +
>  struct TCP_Server_Info {
>  	struct list_head tcp_ses_list;
>  	struct list_head smb_ses_list;
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4f34519..3172baa 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2146,6 +2146,96 @@ cifs_put_tlink(struct tcon_link *tlink)
>  	return;
>  }
>  
> +static inline struct tcon_link *
> +cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb);
> +
> +static int
> +compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
> +{
> +	struct cifs_sb_info *old = CIFS_SB(sb);
> +	struct cifs_sb_info *new = mnt_data->cifs_sb;
> +
> +	if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
> +		return 0;
> +
> +	if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
> +	    (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
> +		return 0;
> +
> +	if (old->rsize != new->rsize)
> +		return 0;
> +
> +	if (new->wsize && new->wsize > old->wsize)
> +		return 0;
> +

...also I think the above condition should be reversed. We don't want
to match if new->wsize is smaller than the existing one. Since the
specified wsize is just a starting point for negotiation, wsize now
means "any wsize less than or equal to this size". If the old->wsize is
bigger than that, then it's outside that range and we shouldn't match.

> +	if (old->mnt_uid != new->mnt_uid || old->mnt_gid != new->mnt_gid)
> +		return 0;
> +
> +	if (old->mnt_file_mode != new->mnt_file_mode ||
> +	    old->mnt_dir_mode != new->mnt_dir_mode)
> +		return 0;
> +
> +	if (strcmp(old->local_nls->charset, new->local_nls->charset))
> +		return 0;
> +
> +	if (old->actimeo != new->actimeo)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +int
> +cifs_match_super(struct super_block *sb, void *data)
> +{
> +	struct cifs_mnt_data *mnt_data = (struct cifs_mnt_data *)data;
> +	struct smb_vol *volume_info;
> +	struct cifs_sb_info *cifs_sb;
> +	struct TCP_Server_Info *tcp_srv;
> +	struct cifsSesInfo *ses;
> +	struct cifsTconInfo *tcon;
> +	struct tcon_link *tlink;
> +	struct sockaddr_storage addr;
> +	int rc = 0;
> +
> +	memset(&addr, 0, sizeof(struct sockaddr_storage));
> +
> +	spin_lock(&cifs_tcp_ses_lock);
> +	cifs_sb = CIFS_SB(sb);
> +	tlink = cifs_get_tlink(cifs_sb_master_tlink(cifs_sb));
> +	if (IS_ERR(tlink)) {
> +		spin_unlock(&cifs_tcp_ses_lock);
> +		return rc;
> +	}
> +	tcon = tlink_tcon(tlink);
> +	ses = tcon->ses;
> +	tcp_srv = ses->server;
> +
> +	volume_info = mnt_data->vol;
> +
> +	if (!volume_info->UNCip || !volume_info->UNC)
> +		goto out;
> +
> +	rc = cifs_fill_sockaddr((struct sockaddr *)&addr,
> +				volume_info->UNCip,
> +				strlen(volume_info->UNCip),
> +				volume_info->port);
> +	if (!rc)
> +		goto out;
> +
> +	if (!match_server(tcp_srv, (struct sockaddr *)&addr, volume_info) ||
> +	    !match_session(ses, volume_info) ||
> +	    !match_tcon(tcon, volume_info->UNC)) {
> +		rc = 0;
> +		goto out;
> +	}
> +
> +	rc = compare_mount_options(sb, mnt_data);
> +out:
> +	cifs_put_tlink(tlink);
> +	spin_unlock(&cifs_tcp_ses_lock);
> +	return rc;
> +}
> +
>  int
>  get_dfs_path(int xid, struct cifsSesInfo *pSesInfo, const char *old_path,
>  	     const struct nls_table *nls_codepage, unsigned int *pnum_referrals,
Steve French May 26, 2011, 4:06 p.m. UTC | #3
On Thu, May 26, 2011 at 10:20 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> On Thu, 26 May 2011 10:02:01 +0400
> Pavel Shilovsky <piastry@etersoft.ru> wrote:
>
>> Add cifs_match_super to use in sget to share superblock between mounts
>> that have the same //server/sharename, credentials and mount options.
>> It helps us to improve performance on work with future SMB2.1 leases.
<snip>
>> +static int
>> +compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
>> +{
>> +     struct cifs_sb_info *old = CIFS_SB(sb);
>> +     struct cifs_sb_info *new = mnt_data->cifs_sb;
>> +
>> +     if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
>> +             return 0;
>> +
>> +     if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
>> +         (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
>> +             return 0;
>> +
>> +     if (old->rsize != new->rsize)
>> +             return 0;
>> +
>> +     if (new->wsize && new->wsize > old->wsize)
>> +             return 0;
>> +
>
> ...also I think the above condition should be reversed. We don't want
> to match if new->wsize is smaller than the existing one. Since the
> specified wsize is just a starting point for negotiation, wsize now
> means "any wsize less than or equal to this size". If the old->wsize is
> bigger than that, then it's outside that range and we shouldn't match.

A more important question is whether the user intentionally tried to
specify a wsize.  I don't see a problem matching a new default mount
request (with no wsize specified, using therefore a default wsize)
with an existing mount (with a larger explicitly specified wsize).
If a user explicitly requests to override to a particular wsize on the 2nd
mount - seems more logical to give them what they ask for if
possible whether larger or smaller.
Pavel Shilovsky May 26, 2011, 4:14 p.m. UTC | #4
2011/5/26 Steve French <smfrench@gmail.com>:
> On Thu, May 26, 2011 at 10:20 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
>> On Thu, 26 May 2011 10:02:01 +0400
>> Pavel Shilovsky <piastry@etersoft.ru> wrote:
>>
>>> Add cifs_match_super to use in sget to share superblock between mounts
>>> that have the same //server/sharename, credentials and mount options.
>>> It helps us to improve performance on work with future SMB2.1 leases.
> <snip>
>>> +static int
>>> +compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
>>> +{
>>> +     struct cifs_sb_info *old = CIFS_SB(sb);
>>> +     struct cifs_sb_info *new = mnt_data->cifs_sb;
>>> +
>>> +     if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
>>> +             return 0;
>>> +
>>> +     if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
>>> +         (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
>>> +             return 0;
>>> +
>>> +     if (old->rsize != new->rsize)
>>> +             return 0;
>>> +
>>> +     if (new->wsize && new->wsize > old->wsize)
>>> +             return 0;
>>> +
>>
>> ...also I think the above condition should be reversed. We don't want
>> to match if new->wsize is smaller than the existing one. Since the
>> specified wsize is just a starting point for negotiation, wsize now
>> means "any wsize less than or equal to this size". If the old->wsize is
>> bigger than that, then it's outside that range and we shouldn't match.
>
> A more important question is whether the user intentionally tried to
> specify a wsize.  I don't see a problem matching a new default mount
> request (with no wsize specified, using therefore a default wsize)
> with an existing mount (with a larger explicitly specified wsize).
> If a user explicitly requests to override to a particular wsize on the 2nd
> mount - seems more logical to give them what they ask for if
> possible whether larger or smaller.
>

If I understand right, Jeff suggested to change wsize logic to: "less
or equal that specified one". In this case we can share any sb that
match this criteria. So, in this case it should be "if (new->wsize &&
new->wsize <= old->wsize) return 0;"
Pavel Shilovsky May 26, 2011, 4:17 p.m. UTC | #5
2011/5/26 Pavel Shilovsky <piastry@etersoft.ru>:
> 2011/5/26 Steve French <smfrench@gmail.com>:
>> On Thu, May 26, 2011 at 10:20 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
>>> On Thu, 26 May 2011 10:02:01 +0400
>>> Pavel Shilovsky <piastry@etersoft.ru> wrote:
>>>
>>>> Add cifs_match_super to use in sget to share superblock between mounts
>>>> that have the same //server/sharename, credentials and mount options.
>>>> It helps us to improve performance on work with future SMB2.1 leases.
>> <snip>
>>>> +static int
>>>> +compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
>>>> +{
>>>> +     struct cifs_sb_info *old = CIFS_SB(sb);
>>>> +     struct cifs_sb_info *new = mnt_data->cifs_sb;
>>>> +
>>>> +     if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
>>>> +             return 0;
>>>> +
>>>> +     if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
>>>> +         (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
>>>> +             return 0;
>>>> +
>>>> +     if (old->rsize != new->rsize)
>>>> +             return 0;
>>>> +
>>>> +     if (new->wsize && new->wsize > old->wsize)
>>>> +             return 0;
>>>> +
>>>
>>> ...also I think the above condition should be reversed. We don't want
>>> to match if new->wsize is smaller than the existing one. Since the
>>> specified wsize is just a starting point for negotiation, wsize now
>>> means "any wsize less than or equal to this size". If the old->wsize is
>>> bigger than that, then it's outside that range and we shouldn't match.
>>
>> A more important question is whether the user intentionally tried to
>> specify a wsize.  I don't see a problem matching a new default mount
>> request (with no wsize specified, using therefore a default wsize)
>> with an existing mount (with a larger explicitly specified wsize).
>> If a user explicitly requests to override to a particular wsize on the 2nd
>> mount - seems more logical to give them what they ask for if
>> possible whether larger or smaller.
>>
>
> If I understand right, Jeff suggested to change wsize logic to: "less
> or equal that specified one". In this case we can share any sb that
> match this criteria. So, in this case it should be "if (new->wsize &&
> new->wsize <= old->wsize) return 0;"

Sorry - new->wsize >= old->wsize
>
> --
> Best regards,
> Pavel Shilovsky.
>
Pavel Shilovsky May 26, 2011, 4:21 p.m. UTC | #6
2011/5/26 Pavel Shilovsky <piastry@etersoft.ru>:
> 2011/5/26 Pavel Shilovsky <piastry@etersoft.ru>:
>> If I understand right, Jeff suggested to change wsize logic to: "less
>> or equal that specified one". In this case we can share any sb that
>> match this criteria. So, in this case it should be "if (new->wsize &&
>> new->wsize <= old->wsize) return 0;"
>
> Sorry - new->wsize >= old->wsize

Sorry again - I confused myself :)

The code should be:
if (new->wsize && new->wsize < old->wsize)
  return 0; /* don't match */
Jeff Layton May 26, 2011, 6:45 p.m. UTC | #7
On Thu, 26 May 2011 11:06:59 -0500
Steve French <smfrench@gmail.com> wrote:

> On Thu, May 26, 2011 at 10:20 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > On Thu, 26 May 2011 10:02:01 +0400
> > Pavel Shilovsky <piastry@etersoft.ru> wrote:
> >
> >> Add cifs_match_super to use in sget to share superblock between mounts
> >> that have the same //server/sharename, credentials and mount options.
> >> It helps us to improve performance on work with future SMB2.1 leases.
> <snip>
> >> +static int
> >> +compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
> >> +{
> >> +     struct cifs_sb_info *old = CIFS_SB(sb);
> >> +     struct cifs_sb_info *new = mnt_data->cifs_sb;
> >> +
> >> +     if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
> >> +             return 0;
> >> +
> >> +     if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
> >> +         (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
> >> +             return 0;
> >> +
> >> +     if (old->rsize != new->rsize)
> >> +             return 0;
> >> +
> >> +     if (new->wsize && new->wsize > old->wsize)
> >> +             return 0;
> >> +
> >
> > ...also I think the above condition should be reversed. We don't want
> > to match if new->wsize is smaller than the existing one. Since the
> > specified wsize is just a starting point for negotiation, wsize now
> > means "any wsize less than or equal to this size". If the old->wsize is
> > bigger than that, then it's outside that range and we shouldn't match.
> 
> A more important question is whether the user intentionally tried to
> specify a wsize.  I don't see a problem matching a new default mount
> request (with no wsize specified, using therefore a default wsize)
> with an existing mount (with a larger explicitly specified wsize).
> If a user explicitly requests to override to a particular wsize on the 2nd
> mount - seems more logical to give them what they ask for if
> possible whether larger or smaller.
> 
> 

Good point. That will probably mean that you'll need to separately
track the "requested" wsize and the "actual" wsize. Honestly though, I
don't see this as a huge issue. I suggest we take this patch as-is and
plan to fix that later.
diff mbox

Patch

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 46fdd55..360fe2e 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -630,6 +630,7 @@  cifs_do_mount(struct file_system_type *fs_type,
 	struct super_block *sb;
 	struct cifs_sb_info *cifs_sb;
 	struct smb_vol *volume_info;
+	struct cifs_mnt_data mnt_data;
 	struct dentry *root;
 
 	cFYI(1, "Devname: %s flags: %d ", dev_name, flags);
@@ -646,12 +647,21 @@  cifs_do_mount(struct file_system_type *fs_type,
 
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
-	sb = sget(fs_type, NULL, set_anon_super, NULL);
+	mnt_data.vol = volume_info;
+	mnt_data.cifs_sb = cifs_sb;
+	mnt_data.flags = flags;
+
+	sb = sget(fs_type, cifs_match_super, set_anon_super, &mnt_data);
 	if (IS_ERR(sb)) {
 		root = ERR_CAST(sb);
 		goto out_cifs_sb;
 	}
 
+	if (sb->s_fs_info) {
+		cFYI(1, "Use existing superblock");
+		goto out_shared;
+	}
+
 	/*
 	 * Copy mount params for use in submounts. Better to do
 	 * the copy here and deal with the error before cleanup gets
@@ -680,9 +690,16 @@  cifs_do_mount(struct file_system_type *fs_type,
 	root = cifs_get_root(volume_info, sb);
 	if (root == NULL)
 		goto out_super;
+
 	cFYI(1, "dentry root is: %p", root);
 	goto out;
 
+out_shared:
+	root = cifs_get_root(volume_info, sb);
+	if (root)
+		cFYI(1, "dentry root is: %p", root);
+	goto out;
+
 out_super:
 	kfree(cifs_sb->mountdata);
 	deactivate_locked_super(sb);
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f57387a..7f90488 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -210,6 +210,25 @@  struct smb_vol {
 	struct nls_table *local_nls;
 };
 
+#define CIFS_MOUNT_MASK (CIFS_MOUNT_NO_PERM | CIFS_MOUNT_SET_UID | \
+			 CIFS_MOUNT_SERVER_INUM | CIFS_MOUNT_DIRECT_IO | \
+			 CIFS_MOUNT_NO_XATTR | CIFS_MOUNT_MAP_SPECIAL_CHR | \
+			 CIFS_MOUNT_UNX_EMUL | CIFS_MOUNT_NO_BRL | \
+			 CIFS_MOUNT_CIFS_ACL | CIFS_MOUNT_OVERR_UID | \
+			 CIFS_MOUNT_OVERR_GID | CIFS_MOUNT_DYNPERM | \
+			 CIFS_MOUNT_NOPOSIXBRL | CIFS_MOUNT_NOSSYNC | \
+			 CIFS_MOUNT_FSCACHE | CIFS_MOUNT_MF_SYMLINKS | \
+			 CIFS_MOUNT_MULTIUSER | CIFS_MOUNT_STRICT_IO)
+
+#define CIFS_MS_MASK (MS_RDONLY | MS_MANDLOCK | MS_NOEXEC | MS_NOSUID | \
+		      MS_NODEV | MS_SYNCHRONOUS)
+
+struct cifs_mnt_data {
+	struct cifs_sb_info *cifs_sb;
+	struct smb_vol *vol;
+	int flags;
+};
+
 struct TCP_Server_Info {
 	struct list_head tcp_ses_list;
 	struct list_head smb_ses_list;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 4f34519..3172baa 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2146,6 +2146,96 @@  cifs_put_tlink(struct tcon_link *tlink)
 	return;
 }
 
+static inline struct tcon_link *
+cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb);
+
+static int
+compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
+{
+	struct cifs_sb_info *old = CIFS_SB(sb);
+	struct cifs_sb_info *new = mnt_data->cifs_sb;
+
+	if ((sb->s_flags & CIFS_MS_MASK) != (mnt_data->flags & CIFS_MS_MASK))
+		return 0;
+
+	if ((old->mnt_cifs_flags & CIFS_MOUNT_MASK) !=
+	    (new->mnt_cifs_flags & CIFS_MOUNT_MASK))
+		return 0;
+
+	if (old->rsize != new->rsize)
+		return 0;
+
+	if (new->wsize && new->wsize > old->wsize)
+		return 0;
+
+	if (old->mnt_uid != new->mnt_uid || old->mnt_gid != new->mnt_gid)
+		return 0;
+
+	if (old->mnt_file_mode != new->mnt_file_mode ||
+	    old->mnt_dir_mode != new->mnt_dir_mode)
+		return 0;
+
+	if (strcmp(old->local_nls->charset, new->local_nls->charset))
+		return 0;
+
+	if (old->actimeo != new->actimeo)
+		return 0;
+
+	return 1;
+}
+
+int
+cifs_match_super(struct super_block *sb, void *data)
+{
+	struct cifs_mnt_data *mnt_data = (struct cifs_mnt_data *)data;
+	struct smb_vol *volume_info;
+	struct cifs_sb_info *cifs_sb;
+	struct TCP_Server_Info *tcp_srv;
+	struct cifsSesInfo *ses;
+	struct cifsTconInfo *tcon;
+	struct tcon_link *tlink;
+	struct sockaddr_storage addr;
+	int rc = 0;
+
+	memset(&addr, 0, sizeof(struct sockaddr_storage));
+
+	spin_lock(&cifs_tcp_ses_lock);
+	cifs_sb = CIFS_SB(sb);
+	tlink = cifs_get_tlink(cifs_sb_master_tlink(cifs_sb));
+	if (IS_ERR(tlink)) {
+		spin_unlock(&cifs_tcp_ses_lock);
+		return rc;
+	}
+	tcon = tlink_tcon(tlink);
+	ses = tcon->ses;
+	tcp_srv = ses->server;
+
+	volume_info = mnt_data->vol;
+
+	if (!volume_info->UNCip || !volume_info->UNC)
+		goto out;
+
+	rc = cifs_fill_sockaddr((struct sockaddr *)&addr,
+				volume_info->UNCip,
+				strlen(volume_info->UNCip),
+				volume_info->port);
+	if (!rc)
+		goto out;
+
+	if (!match_server(tcp_srv, (struct sockaddr *)&addr, volume_info) ||
+	    !match_session(ses, volume_info) ||
+	    !match_tcon(tcon, volume_info->UNC)) {
+		rc = 0;
+		goto out;
+	}
+
+	rc = compare_mount_options(sb, mnt_data);
+out:
+	cifs_put_tlink(tlink);
+	spin_unlock(&cifs_tcp_ses_lock);
+	return rc;
+}
+
 int
 get_dfs_path(int xid, struct cifsSesInfo *pSesInfo, const char *old_path,
 	     const struct nls_table *nls_codepage, unsigned int *pnum_referrals,