smb2 work status

smb2 work status

[Pavel Shilovsky]

Hi all!

Here is my last update of SMB2 code development. What now works:
1) mount/umount.
2) stat, mkdir, rmdir, rm.
3) open/reopen/close, direct read/write.

The work branch is
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev
(rebased to current master).

Your comments are appreciated!

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Pavel Shilovsky]

2011/7/12 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Hi all!
>
> Here is my last update of SMB2 code development. What now works:
> 1) mount/umount.
> 2) stat, mkdir, rmdir, rm.
> 3) open/reopen/close, direct read/write.
>
> The work branch is
> http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev
> (rebased to current master).
>
> Your comments are appreciated!
>
> --
> Best regards,
> Pavel Shilovsky.
>

Jeff, Christoph, can you comment on this, please?

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Christoph Hellwig]

On Sat, Jul 16, 2011 at 01:29:22PM +0400, Pavel Shilovsky wrote:
> Jeff, Christoph, can you comment on this, please?

I really don't like all that ifdef mess.  I'm not quite sure how to
fix in in general, though.  For the inode operations it's fairly simple:
just have a different set for smb2, the only shared code is just the
path_from_dentry call, so there's not much duplication.  But for the
rest I'm not too sure.  Stubbing things out smarter would help, as
would creating more helpers but I'm not sure that's going to help
with everything.  Also some of the big bulk commits earlier in the
series add code that's pretty far from the normal Linux style, e.g.
smb2pdu.c, it would be nice to at least make the new code in cifs
look normal.

Re: smb2 work status

[Pavel Shilovsky]

2011/7/16 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
> On Sat, Jul 16, 2011 at 01:29:22PM +0400, Pavel Shilovsky wrote:
>> Jeff, Christoph, can you comment on this, please?
>
> I really don't like all that ifdef mess.  I'm not quite sure how to
> fix in in general, though.  For the inode operations it's fairly simple:
> just have a different set for smb2, the only shared code is just the
> path_from_dentry call, so there's not much duplication.  But for the
> rest I'm not too sure.  Stubbing things out smarter would help, as
> would creating more helpers but I'm not sure that's going to help
> with everything.  Also some of the big bulk commits earlier in the
> series add code that's pretty far from the normal Linux style, e.g.
> smb2pdu.c, it would be nice to at least make the new code in cifs
> look normal.

Thanks for your comments!

If I understand you right, you mean to separate inode and file
operations for smb2 into a different structures, set them once with
ifdefs in cifs_set_ops() and keep them in smb2inode.c and smb2file.c
files.

As for the code style of new files - I am going to fix it soon.

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Christoph Hellwig]

On Mon, Jul 18, 2011 at 07:45:01PM +0400, Pavel Shilovsky wrote:
> If I understand you right, you mean to separate inode and file
> operations for smb2 into a different structures, set them once with
> ifdefs in cifs_set_ops() and keep them in smb2inode.c and smb2file.c
> files.

That's the idea.  You'll really have to prototype it to check if it
works out nicely.  If we end up duplicating too much it might not
be feasible, but fro ma quick look it should be much better.

Re: smb2 work status

[Jeff Layton]

On Mon, 18 Jul 2011 11:47:30 -0400
Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> On Mon, Jul 18, 2011 at 07:45:01PM +0400, Pavel Shilovsky wrote:
> > If I understand you right, you mean to separate inode and file
> > operations for smb2 into a different structures, set them once with
> > ifdefs in cifs_set_ops() and keep them in smb2inode.c and smb2file.c
> > files.
> 
> That's the idea.  You'll really have to prototype it to check if it
> works out nicely.  If we end up duplicating too much it might not
> be feasible, but fro ma quick look it should be much better.
> 

Agreed. Once you mount, you know what protocol version you're using.
There's little value in continually checking that.

Another idea to consider would be to keep the same set of VFS ops, and
abstract out the lower level set of protocol operations. This is the
model that NFS uses for NFSv2 and 3. v4 is a bit different but it
does use the same file and inode ops for some things. See, for example,
nfs_v3_clientops.

Even if you do take Christoph's suggestion, I think there is value in
abstracting out the protocol operations as well. Better organization
like this will make the code less of a pain to maintain over the long
term.

There are also some things in this set that really ought to be
encapsulated into accessor routines. For example:

+       /* Don't want to modify the buffer as a side effect of this call. */
+       if (server->is_smb2 == false)
+               smb_buffer->smb_buf_length = cpu_to_be32(smb_buf_length);
+#ifdef CONFIG_CIFS_SMB2
+       else
+               smb2_buffer->smb2_buf_length = cpu_to_be32(smb_buf_length);
+#endif


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: smb2 work status

[Pavel Shilovsky]

I updated my branch smb2-dev (with the last changes. I moved to the
idea like you suggested (to keep separate calls for inode and file
operations) and organized the code with callbacks in some places
(cifs_iovec_read, cifs_iovec_write and cifs_reopen_file) that lets us
share the code between both protocols.

Next I am going to rethink cifs_get_inode_info according to this model
too and start to work on buffered i/o file operations. Also, I still
have not fix code style in earlier patches - going to do it too.

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Pavel Shilovsky]

Hi!

I updated my branch smb2-dev
(http://git.etersoft.ru/people/piastry/packages?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev)
with the following changes:
1) rebased to v3.0.
2) buffered i/o support
3) echo support

I used callbacks that let me share the common code for both protocols
in direct i/o, readpage, readpages, writepage, writepages, etc. Also,
I made some minor changes in demultiplex patchset (now smb2-dev branch
is based on this new one).

Comments/thoughts?

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Pavel Shilovsky]

Updated smb2-dev branch
(http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev)
with the following changes:
1) rebased to current master
2) create support
3) readdir support
4) cifs_get_inode_info refactoring

Also, I refactored the code a little - changed statements like
if (server->is_smb2 == false)
    do cifs things
#ifdef CONFIG_CIFS_SMB2
else
    do smb2 things
#endif

to
#ifdef CONFIG_CIFS_SMB2
if (server->is_smb2)
    do smb2 things
else
#endif
    do cifs things

that helps to avoid compiler warnings when compiling without CONFIG_CIFS_SMB2.

Your comments are welcome!

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Pavel Shilovsky]

Updated smb2-dev branch. Changes:
1) get_file_info (and convert smb2_open and smb2_create to use it),
2) rename,
3) hardlink,
4) flush,
5) other cleanups and bugfixes.

Also, I created a mirror repository on git.altlinux.org -
http://git.altlinux.org/people/piastry/public/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev
- due to the problems with git.etersoft.ru.

Next, I am going to start working with oplocks and SMB2.1 leases related code.

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Pavel Shilovsky]

I found out the problem with the kernel hanging. The problem was in
query-info request - we need to set size of our buffer  in a request
but we had sizeof(FILE_ALL_INFO_SMB2) + sizeof(query_req) there. I
removed sizeof(query_req) and expand buffer to store information about
filename - (+ MAX_NAME*2).

So, I rebased my branch with this and other fixes and put it on top of
the current master. As the result, now I have mount/umount and stat
work correctly (according to my stress mount/stat/umount test).

pending cifs patches:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev

my patches:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev-experimental

As the next steps, I think we need to reconsider mid structure - now I
think that having one big structure for both protocols is the best way
to keep code cleaner. Also, I can start implementing inode/file
operations as well.

Comments?

-- 
Best regards,
Pavel Shilovsky.

Re: smb2 work status

[Steve French]

Great - I am looking forward to reviewing these

On Thu, Jun 2, 2011 at 4:10 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi!
>
> I applied pending smb2 patches on top of current master branch and
> created branch smb2-dev:
> http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev
>
> I also created another branch smb2-dev-experimental and put three
> commits on top of smb2-dev - it makes mount/umount work but after some
> period of time kernel crashes - I am working on fixing this now:
> http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev-experimental
>
> Any thoughts/suggestions are welcome!
>
> --
> Best regards,
> Pavel Shilovsky.
>



-- 
Thanks,

Steve

smb2 work status

[Pavel Shilovsky]

Hi!

I applied pending smb2 patches on top of current master branch and
created branch smb2-dev:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev

I also created another branch smb2-dev-experimental and put three
commits on top of smb2-dev - it makes mount/umount work but after some
period of time kernel crashes - I am working on fixing this now:
http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev-experimental

Any thoughts/suggestions are welcome!

-- 
Best regards,
Pavel Shilovsky.