[v3] cifs: Add support of Alternate Data Streams

Commit Message

Shirish Pargaonkar Oct. 10, 2012, 3:11 p.m. UTC

From: Shirish Pargaonkar <[e-mail]>


Add support of Alternate Data Streams (ads).

The generic access flags that cifs client currently employs are sufficient
for alternate data streams as well (MS-CIFS 2.2.4.64.1).

The stream file and stream type are specified using : after the file name,
so that is used to differentiate between a regular file and its
alternate data streams and stream types.
Since they all have the same file id, each path name,
file name:stream name:stream type, has a separate inode with that same
file id but a distinct private data (path name) in that inode to
distinguish them.

This scheme applies only to non-posix compliant servers such as Windows.

One operation that does not work is Rename (0x7).


Signed-off-by: Shirish Pargaonkar <[e-mail]>
---
 fs/cifs/cifsfs.c   |    1 +
 fs/cifs/cifsglob.h |    2 ++
 fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletions(-)

Comments

Jeff Layton Oct. 10, 2012, 4:14 p.m. UTC | #1

On Wed, 10 Oct 2012 10:11:46 -0500
[e-mail] wrote:

> From: Shirish Pargaonkar <[e-mail]>
> 
> 
> Add support of Alternate Data Streams (ads).
> 
> The generic access flags that cifs client currently employs are sufficient
> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
> 
> The stream file and stream type are specified using : after the file name,
> so that is used to differentiate between a regular file and its
> alternate data streams and stream types.
> Since they all have the same file id, each path name,
> file name:stream name:stream type, has a separate inode with that same
> file id but a distinct private data (path name) in that inode to
> distinguish them.
> 
> This scheme applies only to non-posix compliant servers such as Windows.
> 
> One operation that does not work is Rename (0x7).
> 
> 
> Signed-off-by: Shirish Pargaonkar <[e-mail]>
> ---
>  fs/cifs/cifsfs.c   |    1 +
>  fs/cifs/cifsglob.h |    2 ++
>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
>  3 files changed, 35 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index e7931cc..3068992 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
>  {
>  	truncate_inode_pages(&inode->i_data, 0);
>  	clear_inode(inode);
> +	kfree(inode->i_private);
>  	cifs_fscache_release_inode_cookie(inode);
>  }
>  
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f5af252..26d65c7 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
>  #define CIFS_FATTR_DELETE_PENDING	0x2
>  #define CIFS_FATTR_NEED_REVAL		0x4
>  #define CIFS_FATTR_INO_COLLISION	0x8
> +#define CIFS_FATTR_ALTDATASTR		0x10
>  
>  struct cifs_fattr {
>  	u32		cf_flags;
> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
>  	struct timespec	cf_atime;
>  	struct timespec	cf_mtime;
>  	struct timespec	cf_ctime;
> +	char		*cf_private;
>  };
>  
>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index afdff79..93b010b 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>  	struct cifs_fattr fattr;
>  	struct cifs_search_info *srchinf = NULL;
>  
> +	fattr.cf_private = NULL;
>  	tlink = cifs_sb_tlink(cifs_sb);
>  	if (IS_ERR(tlink))
>  		return PTR_ERR(tlink);
> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>  	}
>  
>  	if (!*inode) {
> +		if (strstr(full_path, ":")) {
> +			fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
> +			if (!fattr.cf_private) {
> +				rc = -ENOMEM;
> +				goto cgii_exit;
> +			}
> +			fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
> +		}
> +
>  		*inode = cifs_iget(sb, &fattr);
> -		if (!*inode)
> +		if (*inode) {
> +			if (strstr(full_path, ":") &&
> +					!((*inode)->i_flags & S_PRIVATE)) {
> +				(*inode)->i_private = kstrdup(fattr.cf_private,
> +							GFP_KERNEL);
> +				if ((*inode)->i_private)
> +					(*inode)->i_flags |= S_PRIVATE;
> +				else
> +					rc = -ENOMEM;
> +			}
> +		} else
>  			rc = -ENOMEM;
>  	} else {
>  		cifs_fattr_to_inode(*inode, &fattr);
>  	}
>  
>  cgii_exit:
> +	kfree(fattr.cf_private);
>  	kfree(buf);
>  	cifs_put_tlink(tlink);
>  	return rc;
> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>  	if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
>  		fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
>  
> +	/* looking for an inode of a alternate data stream (full pathname) */
> +	if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
> +		if (!(inode->i_flags & S_PRIVATE)) {
> +			return 0;
> +		} else {
> +			if (strcmp(inode->i_private, fattr->cf_private))
> +				return 0;
> +		}
> +	}
> +
>  	return 1;
>  }
>  

I have real doubts as to whether this patch is necessary. Here's why:

AIUI, the main concern is that you'll open the "normal" data stream on
the file, cache a bunch of data. Then, if you open the alternate
datastream, the kernel will see that as the same inode -- it'll look
like a hardlink. Then when you go to read, you'll get back the cached
data from the original datastream instead of the alternate one.

The key point that's missing here is that servers do not hand out
oplocks for alternate data streams. As long as you've mounted in
cache=strict mode (which is the default for 3.7), then there should be
no problem at all, right?

When you go to open the alternate data stream, it won't matter if the
original datastream had a bunch of data cached. You'll have no oplock
for the file anymore and so you'll if effect be doing DIO to the server
for the alternate datastream anyway.

The only remaining question I have is whether the servers issue oplock
breaks for the "normal" datastream when an alternate datastream is
opened. For instance, suppose I do the following pseudocode:

normal = open("file", ...);

...server issues an oplock for the open. If I then I do:

alternate = open("file:alternate", ...);

...does the server issue an oplock break for the original oplock prior
to performing second open?

Shirish Pargaonkar Oct. 10, 2012, 5:22 p.m. UTC | #2

On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
> On Wed, 10 Oct 2012 10:11:46 -0500
> [e-mail] wrote:
>
>> From: Shirish Pargaonkar <[e-mail]>
>>
>>
>> Add support of Alternate Data Streams (ads).
>>
>> The generic access flags that cifs client currently employs are sufficient
>> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
>>
>> The stream file and stream type are specified using : after the file name,
>> so that is used to differentiate between a regular file and its
>> alternate data streams and stream types.
>> Since they all have the same file id, each path name,
>> file name:stream name:stream type, has a separate inode with that same
>> file id but a distinct private data (path name) in that inode to
>> distinguish them.
>>
>> This scheme applies only to non-posix compliant servers such as Windows.
>>
>> One operation that does not work is Rename (0x7).
>>
>>
>> Signed-off-by: Shirish Pargaonkar <[e-mail]>
>> ---
>>  fs/cifs/cifsfs.c   |    1 +
>>  fs/cifs/cifsglob.h |    2 ++
>>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
>>  3 files changed, 35 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> index e7931cc..3068992 100644
>> --- a/fs/cifs/cifsfs.c
>> +++ b/fs/cifs/cifsfs.c
>> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
>>  {
>>       truncate_inode_pages(&inode->i_data, 0);
>>       clear_inode(inode);
>> +     kfree(inode->i_private);
>>       cifs_fscache_release_inode_cookie(inode);
>>  }
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index f5af252..26d65c7 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
>>  #define CIFS_FATTR_DELETE_PENDING    0x2
>>  #define CIFS_FATTR_NEED_REVAL                0x4
>>  #define CIFS_FATTR_INO_COLLISION     0x8
>> +#define CIFS_FATTR_ALTDATASTR                0x10
>>
>>  struct cifs_fattr {
>>       u32             cf_flags;
>> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
>>       struct timespec cf_atime;
>>       struct timespec cf_mtime;
>>       struct timespec cf_ctime;
>> +     char            *cf_private;
>>  };
>>
>>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index afdff79..93b010b 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>>       struct cifs_fattr fattr;
>>       struct cifs_search_info *srchinf = NULL;
>>
>> +     fattr.cf_private = NULL;
>>       tlink = cifs_sb_tlink(cifs_sb);
>>       if (IS_ERR(tlink))
>>               return PTR_ERR(tlink);
>> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>>       }
>>
>>       if (!*inode) {
>> +             if (strstr(full_path, ":")) {
>> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
>> +                     if (!fattr.cf_private) {
>> +                             rc = -ENOMEM;
>> +                             goto cgii_exit;
>> +                     }
>> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
>> +             }
>> +
>>               *inode = cifs_iget(sb, &fattr);
>> -             if (!*inode)
>> +             if (*inode) {
>> +                     if (strstr(full_path, ":") &&
>> +                                     !((*inode)->i_flags & S_PRIVATE)) {
>> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
>> +                                                     GFP_KERNEL);
>> +                             if ((*inode)->i_private)
>> +                                     (*inode)->i_flags |= S_PRIVATE;
>> +                             else
>> +                                     rc = -ENOMEM;
>> +                     }
>> +             } else
>>                       rc = -ENOMEM;
>>       } else {
>>               cifs_fattr_to_inode(*inode, &fattr);
>>       }
>>
>>  cgii_exit:
>> +     kfree(fattr.cf_private);
>>       kfree(buf);
>>       cifs_put_tlink(tlink);
>>       return rc;
>> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
>>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
>>
>> +     /* looking for an inode of a alternate data stream (full pathname) */
>> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
>> +             if (!(inode->i_flags & S_PRIVATE)) {
>> +                     return 0;
>> +             } else {
>> +                     if (strcmp(inode->i_private, fattr->cf_private))
>> +                             return 0;
>> +             }
>> +     }
>> +
>>       return 1;
>>  }
>>
>
> I have real doubts as to whether this patch is necessary. Here's why:
>
> AIUI, the main concern is that you'll open the "normal" data stream on
> the file, cache a bunch of data. Then, if you open the alternate
> datastream, the kernel will see that as the same inode -- it'll look
> like a hardlink. Then when you go to read, you'll get back the cached
> data from the original datastream instead of the alternate one.
>
> The key point that's missing here is that servers do not hand out
> oplocks for alternate data streams. As long as you've mounted in
> cache=strict mode (which is the default for 3.7), then there should be
> no problem at all, right?

Jeff, where does it say that a server should_not/does_not hand out
oplocks for an alternate data stream?
I have looked for such a documentation and have not found.
The one I read,
 http://msdn.microsoft.com/en-us/library/cc308443.aspx
does not mention/preclude alternate data streams from obtaining oplocks.

>
> When you go to open the alternate data stream, it won't matter if the
> original datastream had a bunch of data cached. You'll have no oplock
> for the file anymore and so you'll if effect be doing DIO to the server
> for the alternate datastream anyway.
>
> The only remaining question I have is whether the servers issue oplock
> breaks for the "normal" datastream when an alternate datastream is
> opened. For instance, suppose I do the following pseudocode:
>
> normal = open("file", ...);
>
> ...server issues an oplock for the open. If I then I do:
>
> alternate = open("file:alternate", ...);
>
> ...does the server issue an oplock break for the original oplock prior
> to performing second open?
>
> --
> Jeff Layton <[e-mail]>

In the above case, server does not issue oplock break for the original oplock.

Regards,

Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton Oct. 10, 2012, 5:25 p.m. UTC | #3

On Wed, 10 Oct 2012 12:22:16 -0500
Shirish Pargaonkar <[e-mail]> wrote:

> On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
> > On Wed, 10 Oct 2012 10:11:46 -0500
> > [e-mail] wrote:
> >
> >> From: Shirish Pargaonkar <[e-mail]>
> >>
> >>
> >> Add support of Alternate Data Streams (ads).
> >>
> >> The generic access flags that cifs client currently employs are sufficient
> >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
> >>
> >> The stream file and stream type are specified using : after the file name,
> >> so that is used to differentiate between a regular file and its
> >> alternate data streams and stream types.
> >> Since they all have the same file id, each path name,
> >> file name:stream name:stream type, has a separate inode with that same
> >> file id but a distinct private data (path name) in that inode to
> >> distinguish them.
> >>
> >> This scheme applies only to non-posix compliant servers such as Windows.
> >>
> >> One operation that does not work is Rename (0x7).
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
> >> ---
> >>  fs/cifs/cifsfs.c   |    1 +
> >>  fs/cifs/cifsglob.h |    2 ++
> >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
> >>  3 files changed, 35 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> index e7931cc..3068992 100644
> >> --- a/fs/cifs/cifsfs.c
> >> +++ b/fs/cifs/cifsfs.c
> >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
> >>  {
> >>       truncate_inode_pages(&inode->i_data, 0);
> >>       clear_inode(inode);
> >> +     kfree(inode->i_private);
> >>       cifs_fscache_release_inode_cookie(inode);
> >>  }
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index f5af252..26d65c7 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
> >>  #define CIFS_FATTR_DELETE_PENDING    0x2
> >>  #define CIFS_FATTR_NEED_REVAL                0x4
> >>  #define CIFS_FATTR_INO_COLLISION     0x8
> >> +#define CIFS_FATTR_ALTDATASTR                0x10
> >>
> >>  struct cifs_fattr {
> >>       u32             cf_flags;
> >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
> >>       struct timespec cf_atime;
> >>       struct timespec cf_mtime;
> >>       struct timespec cf_ctime;
> >> +     char            *cf_private;
> >>  };
> >>
> >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index afdff79..93b010b 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >>       struct cifs_fattr fattr;
> >>       struct cifs_search_info *srchinf = NULL;
> >>
> >> +     fattr.cf_private = NULL;
> >>       tlink = cifs_sb_tlink(cifs_sb);
> >>       if (IS_ERR(tlink))
> >>               return PTR_ERR(tlink);
> >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >>       }
> >>
> >>       if (!*inode) {
> >> +             if (strstr(full_path, ":")) {
> >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
> >> +                     if (!fattr.cf_private) {
> >> +                             rc = -ENOMEM;
> >> +                             goto cgii_exit;
> >> +                     }
> >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
> >> +             }
> >> +
> >>               *inode = cifs_iget(sb, &fattr);
> >> -             if (!*inode)
> >> +             if (*inode) {
> >> +                     if (strstr(full_path, ":") &&
> >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
> >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
> >> +                                                     GFP_KERNEL);
> >> +                             if ((*inode)->i_private)
> >> +                                     (*inode)->i_flags |= S_PRIVATE;
> >> +                             else
> >> +                                     rc = -ENOMEM;
> >> +                     }
> >> +             } else
> >>                       rc = -ENOMEM;
> >>       } else {
> >>               cifs_fattr_to_inode(*inode, &fattr);
> >>       }
> >>
> >>  cgii_exit:
> >> +     kfree(fattr.cf_private);
> >>       kfree(buf);
> >>       cifs_put_tlink(tlink);
> >>       return rc;
> >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
> >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
> >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
> >>
> >> +     /* looking for an inode of a alternate data stream (full pathname) */
> >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
> >> +             if (!(inode->i_flags & S_PRIVATE)) {
> >> +                     return 0;
> >> +             } else {
> >> +                     if (strcmp(inode->i_private, fattr->cf_private))
> >> +                             return 0;
> >> +             }
> >> +     }
> >> +
> >>       return 1;
> >>  }
> >>
> >
> > I have real doubts as to whether this patch is necessary. Here's why:
> >
> > AIUI, the main concern is that you'll open the "normal" data stream on
> > the file, cache a bunch of data. Then, if you open the alternate
> > datastream, the kernel will see that as the same inode -- it'll look
> > like a hardlink. Then when you go to read, you'll get back the cached
> > data from the original datastream instead of the alternate one.
> >
> > The key point that's missing here is that servers do not hand out
> > oplocks for alternate data streams. As long as you've mounted in
> > cache=strict mode (which is the default for 3.7), then there should be
> > no problem at all, right?
> 
> Jeff, where does it say that a server should_not/does_not hand out
> oplocks for an alternate data stream?
> I have looked for such a documentation and have not found.
> The one I read,
>  http://msdn.microsoft.com/en-us/library/cc308443.aspx
> does not mention/preclude alternate data streams from obtaining oplocks.
> 

I believe Jeremy or Chris mentioned it when we were at SDC this year
and I'm cc'ing them here. I don't recall where they said that was
stated.

> >
> > When you go to open the alternate data stream, it won't matter if the
> > original datastream had a bunch of data cached. You'll have no oplock
> > for the file anymore and so you'll if effect be doing DIO to the server
> > for the alternate datastream anyway.
> >
> > The only remaining question I have is whether the servers issue oplock
> > breaks for the "normal" datastream when an alternate datastream is
> > opened. For instance, suppose I do the following pseudocode:
> >
> > normal = open("file", ...);
> >
> > ...server issues an oplock for the open. If I then I do:
> >
> > alternate = open("file:alternate", ...);
> >
> > ...does the server issue an oplock break for the original oplock prior
> > to performing second open?
> >
> > --
> > Jeff Layton <[e-mail]>
> 
> In the above case, server does not issue oplock break for the original oplock.
> 

Pity.

Jeff Layton Oct. 10, 2012, 7:05 p.m. UTC | #4

On Wed, 10 Oct 2012 13:25:59 -0400
Jeff Layton <[e-mail]> wrote:

> On Wed, 10 Oct 2012 12:22:16 -0500
> Shirish Pargaonkar <[e-mail]> wrote:
> 
> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
> > > On Wed, 10 Oct 2012 10:11:46 -0500
> > > [e-mail] wrote:
> > >
> > >> From: Shirish Pargaonkar <[e-mail]>
> > >>
> > >>
> > >> Add support of Alternate Data Streams (ads).
> > >>
> > >> The generic access flags that cifs client currently employs are sufficient
> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
> > >>
> > >> The stream file and stream type are specified using : after the file name,
> > >> so that is used to differentiate between a regular file and its
> > >> alternate data streams and stream types.
> > >> Since they all have the same file id, each path name,
> > >> file name:stream name:stream type, has a separate inode with that same
> > >> file id but a distinct private data (path name) in that inode to
> > >> distinguish them.
> > >>
> > >> This scheme applies only to non-posix compliant servers such as Windows.
> > >>
> > >> One operation that does not work is Rename (0x7).
> > >>
> > >>
> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
> > >> ---
> > >>  fs/cifs/cifsfs.c   |    1 +
> > >>  fs/cifs/cifsglob.h |    2 ++
> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > >> index e7931cc..3068992 100644
> > >> --- a/fs/cifs/cifsfs.c
> > >> +++ b/fs/cifs/cifsfs.c
> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
> > >>  {
> > >>       truncate_inode_pages(&inode->i_data, 0);
> > >>       clear_inode(inode);
> > >> +     kfree(inode->i_private);
> > >>       cifs_fscache_release_inode_cookie(inode);
> > >>  }
> > >>
> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > >> index f5af252..26d65c7 100644
> > >> --- a/fs/cifs/cifsglob.h
> > >> +++ b/fs/cifs/cifsglob.h
> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
> > >>
> > >>  struct cifs_fattr {
> > >>       u32             cf_flags;
> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
> > >>       struct timespec cf_atime;
> > >>       struct timespec cf_mtime;
> > >>       struct timespec cf_ctime;
> > >> +     char            *cf_private;
> > >>  };
> > >>
> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > >> index afdff79..93b010b 100644
> > >> --- a/fs/cifs/inode.c
> > >> +++ b/fs/cifs/inode.c
> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> > >>       struct cifs_fattr fattr;
> > >>       struct cifs_search_info *srchinf = NULL;
> > >>
> > >> +     fattr.cf_private = NULL;
> > >>       tlink = cifs_sb_tlink(cifs_sb);
> > >>       if (IS_ERR(tlink))
> > >>               return PTR_ERR(tlink);
> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> > >>       }
> > >>
> > >>       if (!*inode) {
> > >> +             if (strstr(full_path, ":")) {

		I think you probably want strrchr here. You want to
		search the string backward. It's possible that the
		pathname might have multiple ':' characters in it.

> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);

		Is it really necessary to copy the entire path? This is
		a frequently traveled bit of code, and I think you
		could save some cycles by just copying the alternate
		data stream name.

> > >> +                     if (!fattr.cf_private) {
> > >> +                             rc = -ENOMEM;
> > >> +                             goto cgii_exit;
> > >> +                     }
> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
> > >> +             }
> > >> +
> > >>               *inode = cifs_iget(sb, &fattr);
> > >> -             if (!*inode)
> > >> +             if (*inode) {
> > >> +                     if (strstr(full_path, ":") &&
> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
> > >> +                                                     GFP_KERNEL);
> > >> +                             if ((*inode)->i_private)
> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
> > >> +                             else
> > >> +                                     rc = -ENOMEM;
> > >> +                     }
> > >> +             } else
> > >>                       rc = -ENOMEM;
> > >>       } else {
> > >>               cifs_fattr_to_inode(*inode, &fattr);
> > >>       }
> > >>
> > >>  cgii_exit:
> > >> +     kfree(fattr.cf_private);
> > >>       kfree(buf);
> > >>       cifs_put_tlink(tlink);
> > >>       return rc;
> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
> > >>
> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
> > >> +                     return 0;
> > >> +             } else {
> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
> > >> +                             return 0;
> > >> +             }
> > >> +     }
> > >> +
> > >>       return 1;
> > >>  }
> > >>
> > >


At least in my cursory testing, it's possible to create an alternate
data stream on a directory as well as a file. The resulting inode does
not seem to be treated as a directory, but what are the implications if
a particular server does?

Also, what's the interaction here with "mapchars" mount option? If
someone specifies that, what behavior should they expect wrt alternate
data streams?

> > > I have real doubts as to whether this patch is necessary. Here's why:
> > >
> > > AIUI, the main concern is that you'll open the "normal" data stream on
> > > the file, cache a bunch of data. Then, if you open the alternate
> > > datastream, the kernel will see that as the same inode -- it'll look
> > > like a hardlink. Then when you go to read, you'll get back the cached
> > > data from the original datastream instead of the alternate one.
> > >
> > > The key point that's missing here is that servers do not hand out
> > > oplocks for alternate data streams. As long as you've mounted in
> > > cache=strict mode (which is the default for 3.7), then there should be
> > > no problem at all, right?
> > 
> > Jeff, where does it say that a server should_not/does_not hand out
> > oplocks for an alternate data stream?
> > I have looked for such a documentation and have not found.
> > The one I read,
> >  http://msdn.microsoft.com/en-us/library/cc308443.aspx
> > does not mention/preclude alternate data streams from obtaining oplocks.
> > 
> 
> I believe Jeremy or Chris mentioned it when we were at SDC this year
> and I'm cc'ing them here. I don't recall where they said that was
> stated.
> 

...and that does indeed seem to be incorrect. When testing against
win7, if open an alternate data stream on a file (by passing in a ':'
then I do get an oplock.

I have some comments about the above patch though, see above...

Shirish Pargaonkar Oct. 10, 2012, 7:19 p.m. UTC | #5

On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <[e-mail]> wrote:
> On Wed, 10 Oct 2012 13:25:59 -0400
> Jeff Layton <[e-mail]> wrote:
>
>> On Wed, 10 Oct 2012 12:22:16 -0500
>> Shirish Pargaonkar <[e-mail]> wrote:
>>
>> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
>> > > On Wed, 10 Oct 2012 10:11:46 -0500
>> > > [e-mail] wrote:
>> > >
>> > >> From: Shirish Pargaonkar <[e-mail]>
>> > >>
>> > >>
>> > >> Add support of Alternate Data Streams (ads).
>> > >>
>> > >> The generic access flags that cifs client currently employs are sufficient
>> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
>> > >>
>> > >> The stream file and stream type are specified using : after the file name,
>> > >> so that is used to differentiate between a regular file and its
>> > >> alternate data streams and stream types.
>> > >> Since they all have the same file id, each path name,
>> > >> file name:stream name:stream type, has a separate inode with that same
>> > >> file id but a distinct private data (path name) in that inode to
>> > >> distinguish them.
>> > >>
>> > >> This scheme applies only to non-posix compliant servers such as Windows.
>> > >>
>> > >> One operation that does not work is Rename (0x7).
>> > >>
>> > >>
>> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
>> > >> ---
>> > >>  fs/cifs/cifsfs.c   |    1 +
>> > >>  fs/cifs/cifsglob.h |    2 ++
>> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
>> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
>> > >>
>> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > >> index e7931cc..3068992 100644
>> > >> --- a/fs/cifs/cifsfs.c
>> > >> +++ b/fs/cifs/cifsfs.c
>> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
>> > >>  {
>> > >>       truncate_inode_pages(&inode->i_data, 0);
>> > >>       clear_inode(inode);
>> > >> +     kfree(inode->i_private);
>> > >>       cifs_fscache_release_inode_cookie(inode);
>> > >>  }
>> > >>
>> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> > >> index f5af252..26d65c7 100644
>> > >> --- a/fs/cifs/cifsglob.h
>> > >> +++ b/fs/cifs/cifsglob.h
>> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
>> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
>> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
>> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
>> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
>> > >>
>> > >>  struct cifs_fattr {
>> > >>       u32             cf_flags;
>> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
>> > >>       struct timespec cf_atime;
>> > >>       struct timespec cf_mtime;
>> > >>       struct timespec cf_ctime;
>> > >> +     char            *cf_private;
>> > >>  };
>> > >>
>> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
>> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> > >> index afdff79..93b010b 100644
>> > >> --- a/fs/cifs/inode.c
>> > >> +++ b/fs/cifs/inode.c
>> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> > >>       struct cifs_fattr fattr;
>> > >>       struct cifs_search_info *srchinf = NULL;
>> > >>
>> > >> +     fattr.cf_private = NULL;
>> > >>       tlink = cifs_sb_tlink(cifs_sb);
>> > >>       if (IS_ERR(tlink))
>> > >>               return PTR_ERR(tlink);
>> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> > >>       }
>> > >>
>> > >>       if (!*inode) {
>> > >> +             if (strstr(full_path, ":")) {
>
>                 I think you probably want strrchr here. You want to
>                 search the string backward. It's possible that the
>                 pathname might have multiple ':' characters in it.
>
>> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
>
>                 Is it really necessary to copy the entire path? This is
>                 a frequently traveled bit of code, and I think you
>                 could save some cycles by just copying the alternate
>                 data stream name.
>
>> > >> +                     if (!fattr.cf_private) {
>> > >> +                             rc = -ENOMEM;
>> > >> +                             goto cgii_exit;
>> > >> +                     }
>> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
>> > >> +             }
>> > >> +
>> > >>               *inode = cifs_iget(sb, &fattr);
>> > >> -             if (!*inode)
>> > >> +             if (*inode) {
>> > >> +                     if (strstr(full_path, ":") &&
>> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
>> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
>> > >> +                                                     GFP_KERNEL);
>> > >> +                             if ((*inode)->i_private)
>> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
>> > >> +                             else
>> > >> +                                     rc = -ENOMEM;
>> > >> +                     }
>> > >> +             } else
>> > >>                       rc = -ENOMEM;
>> > >>       } else {
>> > >>               cifs_fattr_to_inode(*inode, &fattr);
>> > >>       }
>> > >>
>> > >>  cgii_exit:
>> > >> +     kfree(fattr.cf_private);
>> > >>       kfree(buf);
>> > >>       cifs_put_tlink(tlink);
>> > >>       return rc;
>> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
>> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
>> > >>
>> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
>> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
>> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
>> > >> +                     return 0;
>> > >> +             } else {
>> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
>> > >> +                             return 0;
>> > >> +             }
>> > >> +     }
>> > >> +
>> > >>       return 1;
>> > >>  }
>> > >>
>> > >
>
>
> At least in my cursory testing, it's possible to create an alternate
> data stream on a directory as well as a file. The resulting inode does
> not seem to be treated as a directory, but what are the implications if
> a particular server does?
>
> Also, what's the interaction here with "mapchars" mount option? If
> someone specifies that, what behavior should they expect wrt alternate
> data streams?
>
>> > > I have real doubts as to whether this patch is necessary. Here's why:
>> > >
>> > > AIUI, the main concern is that you'll open the "normal" data stream on
>> > > the file, cache a bunch of data. Then, if you open the alternate
>> > > datastream, the kernel will see that as the same inode -- it'll look
>> > > like a hardlink. Then when you go to read, you'll get back the cached
>> > > data from the original datastream instead of the alternate one.
>> > >
>> > > The key point that's missing here is that servers do not hand out
>> > > oplocks for alternate data streams. As long as you've mounted in
>> > > cache=strict mode (which is the default for 3.7), then there should be
>> > > no problem at all, right?
>> >
>> > Jeff, where does it say that a server should_not/does_not hand out
>> > oplocks for an alternate data stream?
>> > I have looked for such a documentation and have not found.
>> > The one I read,
>> >  http://msdn.microsoft.com/en-us/library/cc308443.aspx
>> > does not mention/preclude alternate data streams from obtaining oplocks.
>> >
>>
>> I believe Jeremy or Chris mentioned it when we were at SDC this year
>> and I'm cc'ing them here. I don't recall where they said that was
>> stated.
>>
>
> ...and that does indeed seem to be incorrect. When testing against
> win7, if open an alternate data stream on a file (by passing in a ':'
> then I do get an oplock.
>

Jeff,

So I rebooted Windows 2003 server and once it was up,
opened an alternate data stream file, first thing.
So I see notepad asking for both level I and batch oplocks
but server does not grant any.

So an app is free to ask for whatever oplock for a file (including alternate
data stream files) but it is upto the server to grant or deny them.

Then, in case of a linux cifs client asking for level I oplock
for an alternate data stream, it is granted but if linux cifs client
asks for level I and batch oplock, no oplocks are granted.

So, it looks like a client asking for batch oplock on an
alternate data stream file is denied oplocks.
But it is not documented anywhere that I have been able to find.
And I do not know how to make Windows client request only
level I oplock (i.e. no batch oplocks requested) for an open.

I wanted to try similar against a Samba server
(Version 3.6.0-GIT-UNKNOWN-devel), but just have
not been able to make alternate data streams work with
these entries

[smb6]
        path =  /tmp/smb6
        browseable = Yes
        read only = No
        guest account = nobody
        restrict anonymous = 2
        writable = yes
        streams_depot:directory = /tmp/smb6alt
        vfs objects = streams_xattr


> I have some comments about the above patch though, see above...
> --
> Jeff Layton <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shirish Pargaonkar Oct. 10, 2012, 9:40 p.m. UTC | #6

On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <[e-mail]> wrote:
> On Wed, 10 Oct 2012 13:25:59 -0400
> Jeff Layton <[e-mail]> wrote:
>
>> On Wed, 10 Oct 2012 12:22:16 -0500
>> Shirish Pargaonkar <[e-mail]> wrote:
>>
>> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
>> > > On Wed, 10 Oct 2012 10:11:46 -0500
>> > > [e-mail] wrote:
>> > >
>> > >> From: Shirish Pargaonkar <[e-mail]>
>> > >>
>> > >>
>> > >> Add support of Alternate Data Streams (ads).
>> > >>
>> > >> The generic access flags that cifs client currently employs are sufficient
>> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
>> > >>
>> > >> The stream file and stream type are specified using : after the file name,
>> > >> so that is used to differentiate between a regular file and its
>> > >> alternate data streams and stream types.
>> > >> Since they all have the same file id, each path name,
>> > >> file name:stream name:stream type, has a separate inode with that same
>> > >> file id but a distinct private data (path name) in that inode to
>> > >> distinguish them.
>> > >>
>> > >> This scheme applies only to non-posix compliant servers such as Windows.
>> > >>
>> > >> One operation that does not work is Rename (0x7).
>> > >>
>> > >>
>> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
>> > >> ---
>> > >>  fs/cifs/cifsfs.c   |    1 +
>> > >>  fs/cifs/cifsglob.h |    2 ++
>> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
>> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
>> > >>
>> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > >> index e7931cc..3068992 100644
>> > >> --- a/fs/cifs/cifsfs.c
>> > >> +++ b/fs/cifs/cifsfs.c
>> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
>> > >>  {
>> > >>       truncate_inode_pages(&inode->i_data, 0);
>> > >>       clear_inode(inode);
>> > >> +     kfree(inode->i_private);
>> > >>       cifs_fscache_release_inode_cookie(inode);
>> > >>  }
>> > >>
>> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> > >> index f5af252..26d65c7 100644
>> > >> --- a/fs/cifs/cifsglob.h
>> > >> +++ b/fs/cifs/cifsglob.h
>> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
>> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
>> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
>> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
>> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
>> > >>
>> > >>  struct cifs_fattr {
>> > >>       u32             cf_flags;
>> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
>> > >>       struct timespec cf_atime;
>> > >>       struct timespec cf_mtime;
>> > >>       struct timespec cf_ctime;
>> > >> +     char            *cf_private;
>> > >>  };
>> > >>
>> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
>> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> > >> index afdff79..93b010b 100644
>> > >> --- a/fs/cifs/inode.c
>> > >> +++ b/fs/cifs/inode.c
>> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> > >>       struct cifs_fattr fattr;
>> > >>       struct cifs_search_info *srchinf = NULL;
>> > >>
>> > >> +     fattr.cf_private = NULL;
>> > >>       tlink = cifs_sb_tlink(cifs_sb);
>> > >>       if (IS_ERR(tlink))
>> > >>               return PTR_ERR(tlink);
>> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> > >>       }
>> > >>
>> > >>       if (!*inode) {
>> > >> +             if (strstr(full_path, ":")) {
>
>                 I think you probably want strrchr here. You want to
>                 search the string backward. It's possible that the
>                 pathname might have multiple ':' characters in it.
>

I think strchr will work too.

>> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
>
>                 Is it really necessary to copy the entire path? This is
>                 a frequently traveled bit of code, and I think you
>                 could save some cycles by just copying the alternate
>                 data stream name.

yes, that will work.

I can also use strchr as follows

altstr = strchr(full_path, ':');
                if (altstr) {
                        fattr.cf_private = kstrdup(altstr, GFP_KERNEL);

and keep using altstr during rest of the code thus keeping
invocation of string function to just one and using the returned value.

>
>> > >> +                     if (!fattr.cf_private) {
>> > >> +                             rc = -ENOMEM;
>> > >> +                             goto cgii_exit;
>> > >> +                     }
>> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
>> > >> +             }
>> > >> +
>> > >>               *inode = cifs_iget(sb, &fattr);
>> > >> -             if (!*inode)
>> > >> +             if (*inode) {
>> > >> +                     if (strstr(full_path, ":") &&
>> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
>> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
>> > >> +                                                     GFP_KERNEL);
>> > >> +                             if ((*inode)->i_private)
>> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
>> > >> +                             else
>> > >> +                                     rc = -ENOMEM;
>> > >> +                     }
>> > >> +             } else
>> > >>                       rc = -ENOMEM;
>> > >>       } else {
>> > >>               cifs_fattr_to_inode(*inode, &fattr);
>> > >>       }
>> > >>
>> > >>  cgii_exit:
>> > >> +     kfree(fattr.cf_private);
>> > >>       kfree(buf);
>> > >>       cifs_put_tlink(tlink);
>> > >>       return rc;
>> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
>> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
>> > >>
>> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
>> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
>> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
>> > >> +                     return 0;
>> > >> +             } else {
>> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
>> > >> +                             return 0;
>> > >> +             }
>> > >> +     }
>> > >> +
>> > >>       return 1;
>> > >>  }
>> > >>
>> > >
>
>
> At least in my cursory testing, it's possible to create an alternate
> data stream on a directory as well as a file. The resulting inode does
> not seem to be treated as a directory, but what are the implications if
> a particular server does?

Not sure.  I have not found too many usage examples of an
alternate data stream for a directory.  Will do some digging
for info.

>
> Also, what's the interaction here with "mapchars" mount option? If
> someone specifies that, what behavior should they expect wrt alternate
> data streams?

I need to spend some time on this to figure it out.

>
>> > > I have real doubts as to whether this patch is necessary. Here's why:
>> > >
>> > > AIUI, the main concern is that you'll open the "normal" data stream on
>> > > the file, cache a bunch of data. Then, if you open the alternate
>> > > datastream, the kernel will see that as the same inode -- it'll look
>> > > like a hardlink. Then when you go to read, you'll get back the cached
>> > > data from the original datastream instead of the alternate one.
>> > >
>> > > The key point that's missing here is that servers do not hand out
>> > > oplocks for alternate data streams. As long as you've mounted in
>> > > cache=strict mode (which is the default for 3.7), then there should be
>> > > no problem at all, right?
>> >
>> > Jeff, where does it say that a server should_not/does_not hand out
>> > oplocks for an alternate data stream?
>> > I have looked for such a documentation and have not found.
>> > The one I read,
>> >  http://msdn.microsoft.com/en-us/library/cc308443.aspx
>> > does not mention/preclude alternate data streams from obtaining oplocks.
>> >
>>
>> I believe Jeremy or Chris mentioned it when we were at SDC this year
>> and I'm cc'ing them here. I don't recall where they said that was
>> stated.
>>
>
> ...and that does indeed seem to be incorrect. When testing against
> win7, if open an alternate data stream on a file (by passing in a ':'
> then I do get an oplock.
>
> I have some comments about the above patch though, see above...
> --
> Jeff Layton <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeremy Allison Oct. 10, 2012, 10:59 p.m. UTC | #7

On Wed, Oct 10, 2012 at 01:25:59PM -0400, Jeff Layton wrote:
> > 
> > Jeff, where does it say that a server should_not/does_not hand out
> > oplocks for an alternate data stream?
> > I have looked for such a documentation and have not found.
> > The one I read,
> >  http://msdn.microsoft.com/en-us/library/cc308443.aspx
> > does not mention/preclude alternate data streams from obtaining oplocks.
> > 
> 
> I believe Jeremy or Chris mentioned it when we were at SDC this year
> and I'm cc'ing them here. I don't recall where they said that was
> stated.

Hmmm. I don't remember saying that, but the smbtorture tests
will tell you for sure.

Jeremy.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shirish Pargaonkar Oct. 11, 2012, 5:33 a.m. UTC | #8

On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <[e-mail]> wrote:
> On Wed, 10 Oct 2012 13:25:59 -0400
> Jeff Layton <[e-mail]> wrote:
>
>> On Wed, 10 Oct 2012 12:22:16 -0500
>> Shirish Pargaonkar <[e-mail]> wrote:
>>
>> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
>> > > On Wed, 10 Oct 2012 10:11:46 -0500
>> > > [e-mail] wrote:
>> > >
>> > >> From: Shirish Pargaonkar <[e-mail]>
>> > >>
>> > >>
>> > >> Add support of Alternate Data Streams (ads).
>> > >>
>> > >> The generic access flags that cifs client currently employs are sufficient
>> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
>> > >>
>> > >> The stream file and stream type are specified using : after the file name,
>> > >> so that is used to differentiate between a regular file and its
>> > >> alternate data streams and stream types.
>> > >> Since they all have the same file id, each path name,
>> > >> file name:stream name:stream type, has a separate inode with that same
>> > >> file id but a distinct private data (path name) in that inode to
>> > >> distinguish them.
>> > >>
>> > >> This scheme applies only to non-posix compliant servers such as Windows.
>> > >>
>> > >> One operation that does not work is Rename (0x7).
>> > >>
>> > >>
>> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
>> > >> ---
>> > >>  fs/cifs/cifsfs.c   |    1 +
>> > >>  fs/cifs/cifsglob.h |    2 ++
>> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
>> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
>> > >>
>> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > >> index e7931cc..3068992 100644
>> > >> --- a/fs/cifs/cifsfs.c
>> > >> +++ b/fs/cifs/cifsfs.c
>> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
>> > >>  {
>> > >>       truncate_inode_pages(&inode->i_data, 0);
>> > >>       clear_inode(inode);
>> > >> +     kfree(inode->i_private);
>> > >>       cifs_fscache_release_inode_cookie(inode);
>> > >>  }
>> > >>
>> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> > >> index f5af252..26d65c7 100644
>> > >> --- a/fs/cifs/cifsglob.h
>> > >> +++ b/fs/cifs/cifsglob.h
>> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
>> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
>> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
>> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
>> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
>> > >>
>> > >>  struct cifs_fattr {
>> > >>       u32             cf_flags;
>> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
>> > >>       struct timespec cf_atime;
>> > >>       struct timespec cf_mtime;
>> > >>       struct timespec cf_ctime;
>> > >> +     char            *cf_private;
>> > >>  };
>> > >>
>> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
>> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> > >> index afdff79..93b010b 100644
>> > >> --- a/fs/cifs/inode.c
>> > >> +++ b/fs/cifs/inode.c
>> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> > >>       struct cifs_fattr fattr;
>> > >>       struct cifs_search_info *srchinf = NULL;
>> > >>
>> > >> +     fattr.cf_private = NULL;
>> > >>       tlink = cifs_sb_tlink(cifs_sb);
>> > >>       if (IS_ERR(tlink))
>> > >>               return PTR_ERR(tlink);
>> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> > >>       }
>> > >>
>> > >>       if (!*inode) {
>> > >> +             if (strstr(full_path, ":")) {
>
>                 I think you probably want strrchr here. You want to
>                 search the string backward. It's possible that the
>                 pathname might have multiple ':' characters in it.
>
>> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
>
>                 Is it really necessary to copy the entire path? This is
>                 a frequently traveled bit of code, and I think you
>                 could save some cycles by just copying the alternate
>                 data stream name.
>
>> > >> +                     if (!fattr.cf_private) {
>> > >> +                             rc = -ENOMEM;
>> > >> +                             goto cgii_exit;
>> > >> +                     }
>> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
>> > >> +             }
>> > >> +
>> > >>               *inode = cifs_iget(sb, &fattr);
>> > >> -             if (!*inode)
>> > >> +             if (*inode) {
>> > >> +                     if (strstr(full_path, ":") &&
>> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
>> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
>> > >> +                                                     GFP_KERNEL);
>> > >> +                             if ((*inode)->i_private)
>> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
>> > >> +                             else
>> > >> +                                     rc = -ENOMEM;
>> > >> +                     }
>> > >> +             } else
>> > >>                       rc = -ENOMEM;
>> > >>       } else {
>> > >>               cifs_fattr_to_inode(*inode, &fattr);
>> > >>       }
>> > >>
>> > >>  cgii_exit:
>> > >> +     kfree(fattr.cf_private);
>> > >>       kfree(buf);
>> > >>       cifs_put_tlink(tlink);
>> > >>       return rc;
>> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
>> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
>> > >>
>> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
>> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
>> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
>> > >> +                     return 0;
>> > >> +             } else {
>> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
>> > >> +                             return 0;
>> > >> +             }
>> > >> +     }
>> > >> +
>> > >>       return 1;
>> > >>  }
>> > >>
>> > >
>
>
> At least in my cursory testing, it's possible to create an alternate
> data stream on a directory as well as a file. The resulting inode does
> not seem to be treated as a directory, but what are the implications if
> a particular server does?

Which server would that be?  Do you mean like, can you create a
parallel alternate data stream directory which can get populated with
files (with alternate data streams)?  It is probably not possible,
an alternate data stream for a directory is used just the way one
is used for a file.

>
> Also, what's the interaction here with "mapchars" mount option? If
> someone specifies that, what behavior should they expect wrt alternate
> data streams?

It is same as if you were to mount without mapchars mount option.
If you mount with mapchars option and create alternate data streams, they
are accessible only with mapchars mount option.

>
>> > > I have real doubts as to whether this patch is necessary. Here's why:
>> > >
>> > > AIUI, the main concern is that you'll open the "normal" data stream on
>> > > the file, cache a bunch of data. Then, if you open the alternate
>> > > datastream, the kernel will see that as the same inode -- it'll look
>> > > like a hardlink. Then when you go to read, you'll get back the cached
>> > > data from the original datastream instead of the alternate one.
>> > >
>> > > The key point that's missing here is that servers do not hand out
>> > > oplocks for alternate data streams. As long as you've mounted in
>> > > cache=strict mode (which is the default for 3.7), then there should be
>> > > no problem at all, right?
>> >
>> > Jeff, where does it say that a server should_not/does_not hand out
>> > oplocks for an alternate data stream?
>> > I have looked for such a documentation and have not found.
>> > The one I read,
>> >  http://msdn.microsoft.com/en-us/library/cc308443.aspx
>> > does not mention/preclude alternate data streams from obtaining oplocks.
>> >
>>
>> I believe Jeremy or Chris mentioned it when we were at SDC this year
>> and I'm cc'ing them here. I don't recall where they said that was
>> stated.
>>
>
> ...and that does indeed seem to be incorrect. When testing against
> win7, if open an alternate data stream on a file (by passing in a ':'
> then I do get an oplock.
>
> I have some comments about the above patch though, see above...
> --
> Jeff Layton <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton Oct. 11, 2012, 5:54 p.m. UTC | #9

On Thu, 11 Oct 2012 00:33:48 -0500
Shirish Pargaonkar <[e-mail]> wrote:

> On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <[e-mail]> wrote:
> > On Wed, 10 Oct 2012 13:25:59 -0400
> > Jeff Layton <[e-mail]> wrote:
> >
> >> On Wed, 10 Oct 2012 12:22:16 -0500
> >> Shirish Pargaonkar <[e-mail]> wrote:
> >>
> >> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
> >> > > On Wed, 10 Oct 2012 10:11:46 -0500
> >> > > [e-mail] wrote:
> >> > >
> >> > >> From: Shirish Pargaonkar <[e-mail]>
> >> > >>
> >> > >>
> >> > >> Add support of Alternate Data Streams (ads).
> >> > >>
> >> > >> The generic access flags that cifs client currently employs are sufficient
> >> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
> >> > >>
> >> > >> The stream file and stream type are specified using : after the file name,
> >> > >> so that is used to differentiate between a regular file and its
> >> > >> alternate data streams and stream types.
> >> > >> Since they all have the same file id, each path name,
> >> > >> file name:stream name:stream type, has a separate inode with that same
> >> > >> file id but a distinct private data (path name) in that inode to
> >> > >> distinguish them.
> >> > >>
> >> > >> This scheme applies only to non-posix compliant servers such as Windows.
> >> > >>
> >> > >> One operation that does not work is Rename (0x7).
> >> > >>
> >> > >>
> >> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
> >> > >> ---
> >> > >>  fs/cifs/cifsfs.c   |    1 +
> >> > >>  fs/cifs/cifsglob.h |    2 ++
> >> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
> >> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
> >> > >>
> >> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> > >> index e7931cc..3068992 100644
> >> > >> --- a/fs/cifs/cifsfs.c
> >> > >> +++ b/fs/cifs/cifsfs.c
> >> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
> >> > >>  {
> >> > >>       truncate_inode_pages(&inode->i_data, 0);
> >> > >>       clear_inode(inode);
> >> > >> +     kfree(inode->i_private);
> >> > >>       cifs_fscache_release_inode_cookie(inode);
> >> > >>  }
> >> > >>
> >> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> > >> index f5af252..26d65c7 100644
> >> > >> --- a/fs/cifs/cifsglob.h
> >> > >> +++ b/fs/cifs/cifsglob.h
> >> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
> >> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
> >> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
> >> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
> >> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
> >> > >>
> >> > >>  struct cifs_fattr {
> >> > >>       u32             cf_flags;
> >> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
> >> > >>       struct timespec cf_atime;
> >> > >>       struct timespec cf_mtime;
> >> > >>       struct timespec cf_ctime;
> >> > >> +     char            *cf_private;
> >> > >>  };
> >> > >>
> >> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
> >> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> > >> index afdff79..93b010b 100644
> >> > >> --- a/fs/cifs/inode.c
> >> > >> +++ b/fs/cifs/inode.c
> >> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> > >>       struct cifs_fattr fattr;
> >> > >>       struct cifs_search_info *srchinf = NULL;
> >> > >>
> >> > >> +     fattr.cf_private = NULL;
> >> > >>       tlink = cifs_sb_tlink(cifs_sb);
> >> > >>       if (IS_ERR(tlink))
> >> > >>               return PTR_ERR(tlink);
> >> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> > >>       }
> >> > >>
> >> > >>       if (!*inode) {
> >> > >> +             if (strstr(full_path, ":")) {
> >
> >                 I think you probably want strrchr here. You want to
> >                 search the string backward. It's possible that the
> >                 pathname might have multiple ':' characters in it.
> >
> >> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
> >
> >                 Is it really necessary to copy the entire path? This is
> >                 a frequently traveled bit of code, and I think you
> >                 could save some cycles by just copying the alternate
> >                 data stream name.
> >
> >> > >> +                     if (!fattr.cf_private) {
> >> > >> +                             rc = -ENOMEM;
> >> > >> +                             goto cgii_exit;
> >> > >> +                     }
> >> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
> >> > >> +             }
> >> > >> +
> >> > >>               *inode = cifs_iget(sb, &fattr);
> >> > >> -             if (!*inode)
> >> > >> +             if (*inode) {
> >> > >> +                     if (strstr(full_path, ":") &&
> >> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
> >> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
> >> > >> +                                                     GFP_KERNEL);
> >> > >> +                             if ((*inode)->i_private)
> >> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
> >> > >> +                             else
> >> > >> +                                     rc = -ENOMEM;
> >> > >> +                     }
> >> > >> +             } else
> >> > >>                       rc = -ENOMEM;
> >> > >>       } else {
> >> > >>               cifs_fattr_to_inode(*inode, &fattr);
> >> > >>       }
> >> > >>
> >> > >>  cgii_exit:
> >> > >> +     kfree(fattr.cf_private);
> >> > >>       kfree(buf);
> >> > >>       cifs_put_tlink(tlink);
> >> > >>       return rc;
> >> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
> >> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
> >> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
> >> > >>
> >> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
> >> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
> >> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
> >> > >> +                     return 0;
> >> > >> +             } else {
> >> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
> >> > >> +                             return 0;
> >> > >> +             }
> >> > >> +     }
> >> > >> +
> >> > >>       return 1;
> >> > >>  }
> >> > >>
> >> > >
> >
> >
> > At least in my cursory testing, it's possible to create an alternate
> > data stream on a directory as well as a file. The resulting inode does
> > not seem to be treated as a directory, but what are the implications if
> > a particular server does?
> 
> Which server would that be?  Do you mean like, can you create a
> parallel alternate data stream directory which can get populated with
> files (with alternate data streams)?  It is probably not possible,
> an alternate data stream for a directory is used just the way one
> is used for a file.
> 
> >
> > Also, what's the interaction here with "mapchars" mount option? If
> > someone specifies that, what behavior should they expect wrt alternate
> > data streams?
> 
> It is same as if you were to mount without mapchars mount option.
> If you mount with mapchars option and create alternate data streams, they
> are accessible only with mapchars mount option.
> 

':' is one of the characters remapped when you specify mapchars, so I
don't think it's something you can ignore.

Shirish Pargaonkar Oct. 11, 2012, 7:02 p.m. UTC | #10

On Thu, Oct 11, 2012 at 12:54 PM, Jeff Layton <[e-mail]> wrote:
> On Thu, 11 Oct 2012 00:33:48 -0500
> Shirish Pargaonkar <[e-mail]> wrote:
>
>> On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <[e-mail]> wrote:
>> > On Wed, 10 Oct 2012 13:25:59 -0400
>> > Jeff Layton <[e-mail]> wrote:
>> >
>> >> On Wed, 10 Oct 2012 12:22:16 -0500
>> >> Shirish Pargaonkar <[e-mail]> wrote:
>> >>
>> >> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
>> >> > > On Wed, 10 Oct 2012 10:11:46 -0500
>> >> > > [e-mail] wrote:
>> >> > >
>> >> > >> From: Shirish Pargaonkar <[e-mail]>
>> >> > >>
>> >> > >>
>> >> > >> Add support of Alternate Data Streams (ads).
>> >> > >>
>> >> > >> The generic access flags that cifs client currently employs are sufficient
>> >> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
>> >> > >>
>> >> > >> The stream file and stream type are specified using : after the file name,
>> >> > >> so that is used to differentiate between a regular file and its
>> >> > >> alternate data streams and stream types.
>> >> > >> Since they all have the same file id, each path name,
>> >> > >> file name:stream name:stream type, has a separate inode with that same
>> >> > >> file id but a distinct private data (path name) in that inode to
>> >> > >> distinguish them.
>> >> > >>
>> >> > >> This scheme applies only to non-posix compliant servers such as Windows.
>> >> > >>
>> >> > >> One operation that does not work is Rename (0x7).
>> >> > >>
>> >> > >>
>> >> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
>> >> > >> ---
>> >> > >>  fs/cifs/cifsfs.c   |    1 +
>> >> > >>  fs/cifs/cifsglob.h |    2 ++
>> >> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
>> >> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
>> >> > >>
>> >> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> >> > >> index e7931cc..3068992 100644
>> >> > >> --- a/fs/cifs/cifsfs.c
>> >> > >> +++ b/fs/cifs/cifsfs.c
>> >> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
>> >> > >>  {
>> >> > >>       truncate_inode_pages(&inode->i_data, 0);
>> >> > >>       clear_inode(inode);
>> >> > >> +     kfree(inode->i_private);
>> >> > >>       cifs_fscache_release_inode_cookie(inode);
>> >> > >>  }
>> >> > >>
>> >> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> >> > >> index f5af252..26d65c7 100644
>> >> > >> --- a/fs/cifs/cifsglob.h
>> >> > >> +++ b/fs/cifs/cifsglob.h
>> >> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
>> >> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
>> >> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
>> >> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
>> >> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
>> >> > >>
>> >> > >>  struct cifs_fattr {
>> >> > >>       u32             cf_flags;
>> >> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
>> >> > >>       struct timespec cf_atime;
>> >> > >>       struct timespec cf_mtime;
>> >> > >>       struct timespec cf_ctime;
>> >> > >> +     char            *cf_private;
>> >> > >>  };
>> >> > >>
>> >> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
>> >> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> >> > >> index afdff79..93b010b 100644
>> >> > >> --- a/fs/cifs/inode.c
>> >> > >> +++ b/fs/cifs/inode.c
>> >> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> >> > >>       struct cifs_fattr fattr;
>> >> > >>       struct cifs_search_info *srchinf = NULL;
>> >> > >>
>> >> > >> +     fattr.cf_private = NULL;
>> >> > >>       tlink = cifs_sb_tlink(cifs_sb);
>> >> > >>       if (IS_ERR(tlink))
>> >> > >>               return PTR_ERR(tlink);
>> >> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> >> > >>       }
>> >> > >>
>> >> > >>       if (!*inode) {
>> >> > >> +             if (strstr(full_path, ":")) {
>> >
>> >                 I think you probably want strrchr here. You want to
>> >                 search the string backward. It's possible that the
>> >                 pathname might have multiple ':' characters in it.
>> >
>> >> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
>> >
>> >                 Is it really necessary to copy the entire path? This is
>> >                 a frequently traveled bit of code, and I think you
>> >                 could save some cycles by just copying the alternate
>> >                 data stream name.
>> >
>> >> > >> +                     if (!fattr.cf_private) {
>> >> > >> +                             rc = -ENOMEM;
>> >> > >> +                             goto cgii_exit;
>> >> > >> +                     }
>> >> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
>> >> > >> +             }
>> >> > >> +
>> >> > >>               *inode = cifs_iget(sb, &fattr);
>> >> > >> -             if (!*inode)
>> >> > >> +             if (*inode) {
>> >> > >> +                     if (strstr(full_path, ":") &&
>> >> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
>> >> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
>> >> > >> +                                                     GFP_KERNEL);
>> >> > >> +                             if ((*inode)->i_private)
>> >> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
>> >> > >> +                             else
>> >> > >> +                                     rc = -ENOMEM;
>> >> > >> +                     }
>> >> > >> +             } else
>> >> > >>                       rc = -ENOMEM;
>> >> > >>       } else {
>> >> > >>               cifs_fattr_to_inode(*inode, &fattr);
>> >> > >>       }
>> >> > >>
>> >> > >>  cgii_exit:
>> >> > >> +     kfree(fattr.cf_private);
>> >> > >>       kfree(buf);
>> >> > >>       cifs_put_tlink(tlink);
>> >> > >>       return rc;
>> >> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>> >> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
>> >> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
>> >> > >>
>> >> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
>> >> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
>> >> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
>> >> > >> +                     return 0;
>> >> > >> +             } else {
>> >> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
>> >> > >> +                             return 0;
>> >> > >> +             }
>> >> > >> +     }
>> >> > >> +
>> >> > >>       return 1;
>> >> > >>  }
>> >> > >>
>> >> > >
>> >
>> >
>> > At least in my cursory testing, it's possible to create an alternate
>> > data stream on a directory as well as a file. The resulting inode does
>> > not seem to be treated as a directory, but what are the implications if
>> > a particular server does?
>>
>> Which server would that be?  Do you mean like, can you create a
>> parallel alternate data stream directory which can get populated with
>> files (with alternate data streams)?  It is probably not possible,
>> an alternate data stream for a directory is used just the way one
>> is used for a file.
>>
>> >
>> > Also, what's the interaction here with "mapchars" mount option? If
>> > someone specifies that, what behavior should they expect wrt alternate
>> > data streams?
>>
>> It is same as if you were to mount without mapchars mount option.
>> If you mount with mapchars option and create alternate data streams, they
>> are accessible only with mapchars mount option.
>>
>
> ':' is one of the characters remapped when you specify mapchars, so I
> don't think it's something you can ignore.
>
> --
> Jeff Layton <[e-mail]>

Jeff, so they will be separate stream names (one created using
nomapchars (default) mount option and one created using mapchars
mount option) and accessible with respective mount options only
from any client.

I think one more reason for linux cifs client to not even ask for oplocks
irrespective of the server behaviour.

Regards,

Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton Oct. 11, 2012, 7:12 p.m. UTC | #11

On Thu, 11 Oct 2012 14:02:40 -0500
Shirish Pargaonkar <[e-mail]> wrote:

> On Thu, Oct 11, 2012 at 12:54 PM, Jeff Layton <[e-mail]> wrote:
> > On Thu, 11 Oct 2012 00:33:48 -0500
> > Shirish Pargaonkar <[e-mail]> wrote:
> >
> >> On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <[e-mail]> wrote:
> >> > On Wed, 10 Oct 2012 13:25:59 -0400
> >> > Jeff Layton <[e-mail]> wrote:
> >> >
> >> >> On Wed, 10 Oct 2012 12:22:16 -0500
> >> >> Shirish Pargaonkar <[e-mail]> wrote:
> >> >>
> >> >> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <[e-mail]> wrote:
> >> >> > > On Wed, 10 Oct 2012 10:11:46 -0500
> >> >> > > [e-mail] wrote:
> >> >> > >
> >> >> > >> From: Shirish Pargaonkar <[e-mail]>
> >> >> > >>
> >> >> > >>
> >> >> > >> Add support of Alternate Data Streams (ads).
> >> >> > >>
> >> >> > >> The generic access flags that cifs client currently employs are sufficient
> >> >> > >> for alternate data streams as well (MS-CIFS 2.2.4.64.1).
> >> >> > >>
> >> >> > >> The stream file and stream type are specified using : after the file name,
> >> >> > >> so that is used to differentiate between a regular file and its
> >> >> > >> alternate data streams and stream types.
> >> >> > >> Since they all have the same file id, each path name,
> >> >> > >> file name:stream name:stream type, has a separate inode with that same
> >> >> > >> file id but a distinct private data (path name) in that inode to
> >> >> > >> distinguish them.
> >> >> > >>
> >> >> > >> This scheme applies only to non-posix compliant servers such as Windows.
> >> >> > >>
> >> >> > >> One operation that does not work is Rename (0x7).
> >> >> > >>
> >> >> > >>
> >> >> > >> Signed-off-by: Shirish Pargaonkar <[e-mail]>
> >> >> > >> ---
> >> >> > >>  fs/cifs/cifsfs.c   |    1 +
> >> >> > >>  fs/cifs/cifsglob.h |    2 ++
> >> >> > >>  fs/cifs/inode.c    |   33 ++++++++++++++++++++++++++++++++-
> >> >> > >>  3 files changed, 35 insertions(+), 1 deletions(-)
> >> >> > >>
> >> >> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> >> > >> index e7931cc..3068992 100644
> >> >> > >> --- a/fs/cifs/cifsfs.c
> >> >> > >> +++ b/fs/cifs/cifsfs.c
> >> >> > >> @@ -264,6 +264,7 @@ cifs_evict_inode(struct inode *inode)
> >> >> > >>  {
> >> >> > >>       truncate_inode_pages(&inode->i_data, 0);
> >> >> > >>       clear_inode(inode);
> >> >> > >> +     kfree(inode->i_private);
> >> >> > >>       cifs_fscache_release_inode_cookie(inode);
> >> >> > >>  }
> >> >> > >>
> >> >> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> >> > >> index f5af252..26d65c7 100644
> >> >> > >> --- a/fs/cifs/cifsglob.h
> >> >> > >> +++ b/fs/cifs/cifsglob.h
> >> >> > >> @@ -1251,6 +1251,7 @@ struct dfs_info3_param {
> >> >> > >>  #define CIFS_FATTR_DELETE_PENDING    0x2
> >> >> > >>  #define CIFS_FATTR_NEED_REVAL                0x4
> >> >> > >>  #define CIFS_FATTR_INO_COLLISION     0x8
> >> >> > >> +#define CIFS_FATTR_ALTDATASTR                0x10
> >> >> > >>
> >> >> > >>  struct cifs_fattr {
> >> >> > >>       u32             cf_flags;
> >> >> > >> @@ -1268,6 +1269,7 @@ struct cifs_fattr {
> >> >> > >>       struct timespec cf_atime;
> >> >> > >>       struct timespec cf_mtime;
> >> >> > >>       struct timespec cf_ctime;
> >> >> > >> +     char            *cf_private;
> >> >> > >>  };
> >> >> > >>
> >> >> > >>  static inline void free_dfs_info_param(struct dfs_info3_param *param)
> >> >> > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> >> > >> index afdff79..93b010b 100644
> >> >> > >> --- a/fs/cifs/inode.c
> >> >> > >> +++ b/fs/cifs/inode.c
> >> >> > >> @@ -619,6 +619,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> >> > >>       struct cifs_fattr fattr;
> >> >> > >>       struct cifs_search_info *srchinf = NULL;
> >> >> > >>
> >> >> > >> +     fattr.cf_private = NULL;
> >> >> > >>       tlink = cifs_sb_tlink(cifs_sb);
> >> >> > >>       if (IS_ERR(tlink))
> >> >> > >>               return PTR_ERR(tlink);
> >> >> > >> @@ -746,14 +747,34 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> >> > >>       }
> >> >> > >>
> >> >> > >>       if (!*inode) {
> >> >> > >> +             if (strstr(full_path, ":")) {
> >> >
> >> >                 I think you probably want strrchr here. You want to
> >> >                 search the string backward. It's possible that the
> >> >                 pathname might have multiple ':' characters in it.
> >> >
> >> >> > >> +                     fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
> >> >
> >> >                 Is it really necessary to copy the entire path? This is
> >> >                 a frequently traveled bit of code, and I think you
> >> >                 could save some cycles by just copying the alternate
> >> >                 data stream name.
> >> >
> >> >> > >> +                     if (!fattr.cf_private) {
> >> >> > >> +                             rc = -ENOMEM;
> >> >> > >> +                             goto cgii_exit;
> >> >> > >> +                     }
> >> >> > >> +                     fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
> >> >> > >> +             }
> >> >> > >> +
> >> >> > >>               *inode = cifs_iget(sb, &fattr);
> >> >> > >> -             if (!*inode)
> >> >> > >> +             if (*inode) {
> >> >> > >> +                     if (strstr(full_path, ":") &&
> >> >> > >> +                                     !((*inode)->i_flags & S_PRIVATE)) {
> >> >> > >> +                             (*inode)->i_private = kstrdup(fattr.cf_private,
> >> >> > >> +                                                     GFP_KERNEL);
> >> >> > >> +                             if ((*inode)->i_private)
> >> >> > >> +                                     (*inode)->i_flags |= S_PRIVATE;
> >> >> > >> +                             else
> >> >> > >> +                                     rc = -ENOMEM;
> >> >> > >> +                     }
> >> >> > >> +             } else
> >> >> > >>                       rc = -ENOMEM;
> >> >> > >>       } else {
> >> >> > >>               cifs_fattr_to_inode(*inode, &fattr);
> >> >> > >>       }
> >> >> > >>
> >> >> > >>  cgii_exit:
> >> >> > >> +     kfree(fattr.cf_private);
> >> >> > >>       kfree(buf);
> >> >> > >>       cifs_put_tlink(tlink);
> >> >> > >>       return rc;
> >> >> > >> @@ -784,6 +805,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
> >> >> > >>       if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
> >> >> > >>               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
> >> >> > >>
> >> >> > >> +     /* looking for an inode of a alternate data stream (full pathname) */
> >> >> > >> +     if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
> >> >> > >> +             if (!(inode->i_flags & S_PRIVATE)) {
> >> >> > >> +                     return 0;
> >> >> > >> +             } else {
> >> >> > >> +                     if (strcmp(inode->i_private, fattr->cf_private))
> >> >> > >> +                             return 0;
> >> >> > >> +             }
> >> >> > >> +     }
> >> >> > >> +
> >> >> > >>       return 1;
> >> >> > >>  }
> >> >> > >>
> >> >> > >
> >> >
> >> >
> >> > At least in my cursory testing, it's possible to create an alternate
> >> > data stream on a directory as well as a file. The resulting inode does
> >> > not seem to be treated as a directory, but what are the implications if
> >> > a particular server does?
> >>
> >> Which server would that be?  Do you mean like, can you create a
> >> parallel alternate data stream directory which can get populated with
> >> files (with alternate data streams)?  It is probably not possible,
> >> an alternate data stream for a directory is used just the way one
> >> is used for a file.
> >>
> >> >
> >> > Also, what's the interaction here with "mapchars" mount option? If
> >> > someone specifies that, what behavior should they expect wrt alternate
> >> > data streams?
> >>
> >> It is same as if you were to mount without mapchars mount option.
> >> If you mount with mapchars option and create alternate data streams, they
> >> are accessible only with mapchars mount option.
> >>
> >
> > ':' is one of the characters remapped when you specify mapchars, so I
> > don't think it's something you can ignore.
> >
> > --
> > Jeff Layton <[e-mail]>
> 
> Jeff, so they will be separate stream names (one created using
> nomapchars (default) mount option and one created using mapchars
> mount option) and accessible with respective mount options only
> from any client.
> 
> I think one more reason for linux cifs client to not even ask for oplocks
> irrespective of the server behaviour.
> 

I think you misunderstand me...

Your patch is looking for ':' characters in the filename of the local
charset and treating those files in a special way. There is no guarantee
though that that filename will have a ':' in it once you send it to the
server if mapchars is in effect. If it doesn't then you will no longer
be dealing with an alternate data stream. Your current patch does not
account for that, AFAICT...

Patch

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index e7931cc..3068992 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -264,6 +264,7 @@  cifs_evict_inode(struct inode *inode)
 {
 	truncate_inode_pages(&inode->i_data, 0);
 	clear_inode(inode);
+	kfree(inode->i_private);
 	cifs_fscache_release_inode_cookie(inode);
 }
 
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f5af252..26d65c7 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1251,6 +1251,7 @@  struct dfs_info3_param {
 #define CIFS_FATTR_DELETE_PENDING	0x2
 #define CIFS_FATTR_NEED_REVAL		0x4
 #define CIFS_FATTR_INO_COLLISION	0x8
+#define CIFS_FATTR_ALTDATASTR		0x10
 
 struct cifs_fattr {
 	u32		cf_flags;
@@ -1268,6 +1269,7 @@  struct cifs_fattr {
 	struct timespec	cf_atime;
 	struct timespec	cf_mtime;
 	struct timespec	cf_ctime;
+	char		*cf_private;
 };
 
 static inline void free_dfs_info_param(struct dfs_info3_param *param)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index afdff79..93b010b 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -619,6 +619,7 @@  cifs_get_inode_info(struct inode **inode, const char *full_path,
 	struct cifs_fattr fattr;
 	struct cifs_search_info *srchinf = NULL;
 
+	fattr.cf_private = NULL;
 	tlink = cifs_sb_tlink(cifs_sb);
 	if (IS_ERR(tlink))
 		return PTR_ERR(tlink);
@@ -746,14 +747,34 @@  cifs_get_inode_info(struct inode **inode, const char *full_path,
 	}
 
 	if (!*inode) {
+		if (strstr(full_path, ":")) {
+			fattr.cf_private = kstrdup(full_path, GFP_KERNEL);
+			if (!fattr.cf_private) {
+				rc = -ENOMEM;
+				goto cgii_exit;
+			}
+			fattr.cf_flags |= CIFS_FATTR_ALTDATASTR;
+		}
+
 		*inode = cifs_iget(sb, &fattr);
-		if (!*inode)
+		if (*inode) {
+			if (strstr(full_path, ":") &&
+					!((*inode)->i_flags & S_PRIVATE)) {
+				(*inode)->i_private = kstrdup(fattr.cf_private,
+							GFP_KERNEL);
+				if ((*inode)->i_private)
+					(*inode)->i_flags |= S_PRIVATE;
+				else
+					rc = -ENOMEM;
+			}
+		} else
 			rc = -ENOMEM;
 	} else {
 		cifs_fattr_to_inode(*inode, &fattr);
 	}
 
 cgii_exit:
+	kfree(fattr.cf_private);
 	kfree(buf);
 	cifs_put_tlink(tlink);
 	return rc;
@@ -784,6 +805,16 @@  cifs_find_inode(struct inode *inode, void *opaque)
 	if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry))
 		fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
 
+	/* looking for an inode of a alternate data stream (full pathname) */
+	if (fattr->cf_flags & CIFS_FATTR_ALTDATASTR) {
+		if (!(inode->i_flags & S_PRIVATE)) {
+			return 0;
+		} else {
+			if (strcmp(inode->i_private, fattr->cf_private))
+				return 0;
+		}
+	}
+
 	return 1;
 }