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