[PATCH] copy-chunk fails if src and dst file are on different tcons

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
Hi!

As the subject says, this fails against Samba but works against
Windows. Verified against Windows 2016.

Attached is a possible fix plus test.

Please review & push if happy. Thanks!

-slow

bug12752-master.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 12:55:55PM +0200, Ralph Böhme via samba-technical wrote:
> Hi!
>
> As the subject says, this fails against Samba but works against
> Windows. Verified against Windows 2016.
>
> Attached is a possible fix plus test.
>
> Please review & push if happy. Thanks!

please ignore for now. metze raised an issue over private RPC dealing with the
VFS handles that we end up using in the send functions.

-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 04:52:10PM +0200, Ralph Böhme wrote:

> On Fri, Apr 21, 2017 at 12:55:55PM +0200, Ralph Böhme via samba-technical wrote:
> > Hi!
> >
> > As the subject says, this fails against Samba but works against
> > Windows. Verified against Windows 2016.
> >
> > Attached is a possible fix plus test.
> >
> > Please review & push if happy. Thanks!
>
> please ignore for now. metze raised an issue over private RPC dealing with the
> VFS handles that we end up using in the send functions.
metze, maybe we can just do the check at the vfs layer. See attached patch for
the basic idea. Should be expanded to cover all vfs functions that take a fsp
and a vfs handle.

Just fetching and "looking" at an fsp at the smb layer should be possible, it's
where we start using it where we should check. Or am I still missing something?

-slow
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 06:39:38PM +0200, Ralph Böhme via samba-technical wrote:

> On Fri, Apr 21, 2017 at 04:52:10PM +0200, Ralph Böhme wrote:
> > On Fri, Apr 21, 2017 at 12:55:55PM +0200, Ralph Böhme via samba-technical wrote:
> > > Hi!
> > >
> > > As the subject says, this fails against Samba but works against
> > > Windows. Verified against Windows 2016.
> > >
> > > Attached is a possible fix plus test.
> > >
> > > Please review & push if happy. Thanks!
> >
> > please ignore for now. metze raised an issue over private RPC dealing with the
> > VFS handles that we end up using in the send functions.
>
> metze, maybe we can just do the check at the vfs layer. See attached patch for
> the basic idea. Should be expanded to cover all vfs functions that take a fsp
> and a vfs handle.
>
> Just fetching and "looking" at an fsp at the smb layer should be possible, it's
> where we start using it where we should check. Or am I still missing something?

Just my 2cents. This looks like the right approach but does mean
changing all of the fsp-based VFS calls. Is there
any way to do this above the VFS layer (haven't thought too
deeply about this yet) ?

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 02:21:41PM -0700, Jeremy Allison wrote:

> On Fri, Apr 21, 2017 at 06:39:38PM +0200, Ralph Böhme via samba-technical wrote:
> > On Fri, Apr 21, 2017 at 04:52:10PM +0200, Ralph Böhme wrote:
> > > On Fri, Apr 21, 2017 at 12:55:55PM +0200, Ralph Böhme via samba-technical wrote:
> > > > Hi!
> > > >
> > > > As the subject says, this fails against Samba but works against
> > > > Windows. Verified against Windows 2016.
> > > >
> > > > Attached is a possible fix plus test.
> > > >
> > > > Please review & push if happy. Thanks!
> > >
> > > please ignore for now. metze raised an issue over private RPC dealing with the
> > > VFS handles that we end up using in the send functions.
> >
> > metze, maybe we can just do the check at the vfs layer. See attached patch for
> > the basic idea. Should be expanded to cover all vfs functions that take a fsp
> > and a vfs handle.
> >
> > Just fetching and "looking" at an fsp at the smb layer should be possible, it's
> > where we start using it where we should check. Or am I still missing something?
>
> Just my 2cents. This looks like the right approach but does mean
> changing all of the fsp-based VFS calls.

d'oh! Most VFS macros that take an fsp don't take a conn directly anyway, instead
the macro pulls the vfs handle out of the fsp (fsp->conn->vfs_handles). Which is
exactly what we want.

There are a few VFS macros that don't adhere to this scheme (eg
SMB_VFS_GET_ALLOC_SIZE, SMB_VFS_STREAMINFO, SMB_VFS_STRICT_LOCK,
SMB_VFS_COPY_CHUNK_SEND), but I guess that's not an purpose and could be fixed.

Thinking more about it, my change would result in VFS calls running with their
curdir set to a different tcon then their fsp. We currently rely on doing a
chdir to the share root when processing an smb req for a tcon, don't we? This
might actually sink my ship. I imagine shadow_copy2 might not be happy when
processing a request to read from an fsp that is assodicated with tcon 1 while
the curdir is the share root of tcon 2. Hm.

> Is there any way to do this above the VFS layer (haven't thought too deeply
> about this yet) ?

Short of adding special casing this by adding a read/write loop at the smb
layer: I don't think so.

-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Sat, Apr 22, 2017 at 02:36:35PM +0200, Ralph Böhme wrote:

> On Fri, Apr 21, 2017 at 02:21:41PM -0700, Jeremy Allison wrote:
> > On Fri, Apr 21, 2017 at 06:39:38PM +0200, Ralph Böhme via samba-technical wrote:
> > > On Fri, Apr 21, 2017 at 04:52:10PM +0200, Ralph Böhme wrote:
> > > > On Fri, Apr 21, 2017 at 12:55:55PM +0200, Ralph Böhme via samba-technical wrote:
> > > > > Hi!
> > > > >
> > > > > As the subject says, this fails against Samba but works against
> > > > > Windows. Verified against Windows 2016.
> > > > >
> > > > > Attached is a possible fix plus test.
> > > > >
> > > > > Please review & push if happy. Thanks!
> > > >
> > > > please ignore for now. metze raised an issue over private RPC dealing with the
> > > > VFS handles that we end up using in the send functions.
> > >
> > > metze, maybe we can just do the check at the vfs layer. See attached patch for
> > > the basic idea. Should be expanded to cover all vfs functions that take a fsp
> > > and a vfs handle.
> > >
> > > Just fetching and "looking" at an fsp at the smb layer should be possible, it's
> > > where we start using it where we should check. Or am I still missing something?
> >
> > Just my 2cents. This looks like the right approach but does mean
> > changing all of the fsp-based VFS calls.
>
> d'oh! Most VFS macros that take an fsp don't take a conn directly anyway, instead
> the macro pulls the vfs handle out of the fsp (fsp->conn->vfs_handles). Which is
> exactly what we want.
>
> There are a few VFS macros that don't adhere to this scheme (eg
> SMB_VFS_GET_ALLOC_SIZE, SMB_VFS_STREAMINFO, SMB_VFS_STRICT_LOCK,
> SMB_VFS_COPY_CHUNK_SEND), but I guess that's not an purpose and could be fixed.
>
> Thinking more about it, my change would result in VFS calls running with their
> curdir set to a different tcon then their fsp. We currently rely on doing a
> chdir to the share root when processing an smb req for a tcon, don't we? This
> might actually sink my ship. I imagine shadow_copy2 might not be happy when
> processing a request to read from an fsp that is assodicated with tcon 1 while
> the curdir is the share root of tcon 2. Hm.
so attached is a patchset that adds some magic to the relevant VFS functions
(copy-chunk) that allows them to work across shares -- something that Windows
supports.

The basic idea is to associate a token/cookie with an fsp in the VFS module
function that sees the source fsp (so the resume-key ioctl in case of
copy-chunk) and then fetch the source fsp from the cookie again later.

To generalize on this mechanism and to prepare for future support of ODX
(Windows Offloaded Data Transfer) I'm consolidating the VFS interface on two
token based VFS function: OFFLOAD_READ and OFFLOAD_WRITE.

For now I'm essentially renaming COPY_CHUNK adding the additional fsp/cookie
magic to allow for copied across shares. SMB layer support for ODX is not yet
implemented. After some research it seems there is some support in the Linux
kernel already, exposed via ioctl()s, but we'd need some real ODX capable
hardware to implement this in Samba.

Additional reviewers *highly* welcome!

Cheerio!
-slow

offload-read-write.patch (180K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Fri, Jun 30, 2017 at 08:07:56PM +0200, Ralph Böhme via samba-technical wrote:

> On Sat, Apr 22, 2017 at 02:36:35PM +0200, Ralph Böhme wrote:
> > On Fri, Apr 21, 2017 at 02:21:41PM -0700, Jeremy Allison wrote:
> > > On Fri, Apr 21, 2017 at 06:39:38PM +0200, Ralph Böhme via samba-technical wrote:
> > > > On Fri, Apr 21, 2017 at 04:52:10PM +0200, Ralph Böhme wrote:
> > > > > On Fri, Apr 21, 2017 at 12:55:55PM +0200, Ralph Böhme via samba-technical wrote:
> > > > > > Hi!
> > > > > >
> > > > > > As the subject says, this fails against Samba but works against
> > > > > > Windows. Verified against Windows 2016.
> > > > > >
> > > > > > Attached is a possible fix plus test.
> > > > > >
> > > > > > Please review & push if happy. Thanks!
> > > > >
> > > > > please ignore for now. metze raised an issue over private RPC dealing with the
> > > > > VFS handles that we end up using in the send functions.
> > > >
> > > > metze, maybe we can just do the check at the vfs layer. See attached patch for
> > > > the basic idea. Should be expanded to cover all vfs functions that take a fsp
> > > > and a vfs handle.
> > > >
> > > > Just fetching and "looking" at an fsp at the smb layer should be possible, it's
> > > > where we start using it where we should check. Or am I still missing something?
> > >
> > > Just my 2cents. This looks like the right approach but does mean
> > > changing all of the fsp-based VFS calls.
> >
> > d'oh! Most VFS macros that take an fsp don't take a conn directly anyway, instead
> > the macro pulls the vfs handle out of the fsp (fsp->conn->vfs_handles). Which is
> > exactly what we want.
> >
> > There are a few VFS macros that don't adhere to this scheme (eg
> > SMB_VFS_GET_ALLOC_SIZE, SMB_VFS_STREAMINFO, SMB_VFS_STRICT_LOCK,
> > SMB_VFS_COPY_CHUNK_SEND), but I guess that's not an purpose and could be fixed.
> >
> > Thinking more about it, my change would result in VFS calls running with their
> > curdir set to a different tcon then their fsp. We currently rely on doing a
> > chdir to the share root when processing an smb req for a tcon, don't we? This
> > might actually sink my ship. I imagine shadow_copy2 might not be happy when
> > processing a request to read from an fsp that is assodicated with tcon 1 while
> > the curdir is the share root of tcon 2. Hm.
>
> so attached is a patchset that adds some magic to the relevant VFS functions
> (copy-chunk) that allows them to work across shares -- something that Windows
> supports.
>
> The basic idea is to associate a token/cookie with an fsp in the VFS module
> function that sees the source fsp (so the resume-key ioctl in case of
> copy-chunk) and then fetch the source fsp from the cookie again later.
>
> To generalize on this mechanism and to prepare for future support of ODX
> (Windows Offloaded Data Transfer) I'm consolidating the VFS interface on two
> token based VFS function: OFFLOAD_READ and OFFLOAD_WRITE.
>
> For now I'm essentially renaming COPY_CHUNK adding the additional fsp/cookie
> magic to allow for copied across shares. SMB layer support for ODX is not yet
> implemented. After some research it seems there is some support in the Linux
> kernel already, exposed via ioctl()s, but we'd need some real ODX capable
> hardware to implement this in Samba.

just pushed this to autobuild.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Mon, 3 Jul 2017 11:19:08 +0200, Ralph Böhme via samba-technical wrote:

> just pushed this to autobuild.

Thanks for your work on this, Ralph. I do have one concern though - I
see one regression when running the fs_specific dup_extents_src_lock and
dup_extents_dest_lock tests atop btrfs (which supports dup-extents):

failure: samba3.smb2.ioctl fs_specific.dup_extents_src_lock(nt4_dc) [
Exception: Exception: ../source4/torture/smb2/ioctl.c:6506: status was NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK: FSCTL_DUP_EXTENTS_TO_FILE unlocked
...
failure: samba3.smb2.ioctl fs_specific.dup_extents_dest_lock(nt4_dc) [
Exception: Exception: ../source4/torture/smb2/ioctl.c:6612: status was NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK: FSCTL_DUP_EXTENTS_TO_FILE unlocked

I'd prefer if this was fixed before the patchset goes in. I've only
skimmed through the rest of the changes, but they look good on first
impression.

Cheers, David

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Mon, Jul 03, 2017 at 02:25:05PM +0200, David Disseldorp wrote:

> On Mon, 3 Jul 2017 11:19:08 +0200, Ralph Böhme via samba-technical wrote:
>
> > just pushed this to autobuild.
>
> Thanks for your work on this, Ralph. I do have one concern though - I
> see one regression when running the fs_specific dup_extents_src_lock and
> dup_extents_dest_lock tests atop btrfs (which supports dup-extents):
>
> failure: samba3.smb2.ioctl fs_specific.dup_extents_src_lock(nt4_dc) [
> Exception: Exception: ../source4/torture/smb2/ioctl.c:6506: status was NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK: FSCTL_DUP_EXTENTS_TO_FILE unlocked
> ...
> failure: samba3.smb2.ioctl fs_specific.dup_extents_dest_lock(nt4_dc) [
> Exception: Exception: ../source4/torture/smb2/ioctl.c:6612: status was NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK: FSCTL_DUP_EXTENTS_TO_FILE unlocked
>
> I'd prefer if this was fixed before the patchset goes in. I've only
> skimmed through the rest of the changes, but they look good on first
> impression.

lemme check. I did some manual tests with btfs, but I didn't run all tests. I
cancelled the autobuild.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Mon, Jul 03, 2017 at 02:32:26PM +0200, Ralph Böhme wrote:

> On Mon, Jul 03, 2017 at 02:25:05PM +0200, David Disseldorp wrote:
> > On Mon, 3 Jul 2017 11:19:08 +0200, Ralph Böhme via samba-technical wrote:
> >
> > > just pushed this to autobuild.
> >
> > Thanks for your work on this, Ralph. I do have one concern though - I
> > see one regression when running the fs_specific dup_extents_src_lock and
> > dup_extents_dest_lock tests atop btrfs (which supports dup-extents):
> >
> > failure: samba3.smb2.ioctl fs_specific.dup_extents_src_lock(nt4_dc) [
> > Exception: Exception: ../source4/torture/smb2/ioctl.c:6506: status was NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK: FSCTL_DUP_EXTENTS_TO_FILE unlocked
> > ...
> > failure: samba3.smb2.ioctl fs_specific.dup_extents_dest_lock(nt4_dc) [
> > Exception: Exception: ../source4/torture/smb2/ioctl.c:6612: status was NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK: FSCTL_DUP_EXTENTS_TO_FILE unlocked
> >
> > I'd prefer if this was fixed before the patchset goes in. I've only
> > skimmed through the rest of the changes, but they look good on first
> > impression.
>
> lemme check. I did some manual tests with btfs, but I didn't run all tests. I
> cancelled the autobuild.
woo, thanks a lot for spotting this!

Turns out there was a simple logic flaw in the offload token DB: trying to
insert a token that was already stored was not allowed. That of course breaks
the permissible SMB2 behaviour of calling the fetch-resume-key ioctl twice for
the same filehandle, or, in the failing btrfs test case, calling the dup-extents
ioctl twice on the same file-handle.

Attached is an updated patchset, I'm also attaching the diff-diff. This one
passes the dup-extents smbtorture tests on a btrfs fs.

metze, can you please (re-)review the commits marked as TODO/REVIEW? Thanks!

Cheerio!
-slow

offload-read-write-v2.patch (184K) Download Attachment
diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Mon, 3 Jul 2017 15:26:24 +0200, Ralph Böhme wrote:

> > > see one regression when running the fs_specific dup_extents_src_lock and
> > > dup_extents_dest_lock tests atop btrfs (which supports dup-extents):
...

> woo, thanks a lot for spotting this!
>
> Turns out there was a simple logic flaw in the offload token DB: trying to
> insert a token that was already stored was not allowed. That of course breaks
> the permissible SMB2 behaviour of calling the fetch-resume-key ioctl twice for
> the same filehandle, or, in the failing btrfs test case, calling the dup-extents
> ioctl twice on the same file-handle.
>
> Attached is an updated patchset, I'm also attaching the diff-diff. This one
> passes the dup-extents smbtorture tests on a btrfs fs.

Thanks for the follow-up! The diff-diff looks fine. I'll leave the final
ack up to Metze.

Cheers, David

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] copy-chunk fails if src and dst file are on different tcons

Samba - samba-technical mailing list
On Mon, Jul 03, 2017 at 03:41:02PM +0200, David Disseldorp wrote:

> On Mon, 3 Jul 2017 15:26:24 +0200, Ralph Böhme wrote:
>
> > > > see one regression when running the fs_specific dup_extents_src_lock and
> > > > dup_extents_dest_lock tests atop btrfs (which supports dup-extents):
> ...
> > woo, thanks a lot for spotting this!
> >
> > Turns out there was a simple logic flaw in the offload token DB: trying to
> > insert a token that was already stored was not allowed. That of course breaks
> > the permissible SMB2 behaviour of calling the fetch-resume-key ioctl twice for
> > the same filehandle, or, in the failing btrfs test case, calling the dup-extents
> > ioctl twice on the same file-handle.
> >
> > Attached is an updated patchset, I'm also attaching the diff-diff. This one
> > passes the dup-extents smbtorture tests on a btrfs fs.
>
> Thanks for the follow-up! The diff-diff looks fine. I'll leave the final
> ack up to Metze.

thanks! metze acked, so I'm going to push later on.

Cheerio!
-slow