On Wed, Oct 10, 2012 at 2:05 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Wed, 10 Oct 2012 13:25:59 -0400
> Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
>> On Wed, 10 Oct 2012 12:22:16 -0500
>> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>>
>> > On Wed, Oct 10, 2012 at 11:14 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> > > On Wed, 10 Oct 2012 10:11:46 -0500
>> > > shirishpargaonkar@xxxxxxxxx wrote:
>> > >
>> > >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> > >>
>> > >>
>> > >> 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 <shirishpargaonkar@xxxxxxxxx>
>> > >> ---
>> > >> 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 <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
