Quantcast

[PATCH] Fix the new async copy-chunk for streams

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Fix the new async copy-chunk for streams

Samba - samba-technical mailing list
Hi!

A few days ago I noticed that a recent macOS client requests copy-chunk of named
streams and we fail miserably. That's my fault: since making copy-chunk async
[1] in vfs_default, we use the async pread/pwrite VFS versions to do the actual
copy. :/

With vfs_streams_xattr this silently copies data in the default data stream, not
in the requested named stream, so the copy-chunk seems to succeed.

After requesting the copy-chunk, the client does a setinfo(size) on the stream,
so after this the stream has the correct size, alas it's all 0 -- voila, silent
(meta)data corruption.

Luckily the new async copy-chunk is only in master, so even though I filed a
bugreport, we don't need to backport and the alarm whistel can stay off. *phew*

The attached patch adds implementations of async pread and pwrite to
vfs_streams_xattr and vfs_fruit. vfs_streams_depot doesn't need changes, as it
works correctly with the vfs_default implementation of async pread/pwrite as it
provides usable fds.

Btw, with these changes in place, the only thing missing for allowing read/write aio
on named streams is flush_send/recv. Or was there any other reason for not
allowing aio on named streams?

Fwiw, a Windows server supports copy-chunk of named streams.

Please review & push if ok. Thanks!

-slow

[1] 60e45a2d25401eaf9a15a86d19114670ccfde259

bug12787-master.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix the new async copy-chunk for streams

Samba - samba-technical mailing list
Hi Ralph,

On Sat, 13 May 2017 16:06:03 +0200, Ralph Böhme via samba-technical wrote:

> Hi!
>
> A few days ago I noticed that a recent macOS client requests copy-chunk of named
> streams and we fail miserably. That's my fault: since making copy-chunk async
> [1] in vfs_default, we use the async pread/pwrite VFS versions to do the actual
> copy. :/
>
> With vfs_streams_xattr this silently copies data in the default data stream, not
> in the requested named stream, so the copy-chunk seems to succeed.
>
> After requesting the copy-chunk, the client does a setinfo(size) on the stream,
> so after this the stream has the correct size, alas it's all 0 -- voila, silent
> (meta)data corruption.

Ouch, thanks for catching this.

> Luckily the new async copy-chunk is only in master, so even though I filed a
> bugreport, we don't need to backport and the alarm whistel can stay off. *phew*
>
> The attached patch adds implementations of async pread and pwrite to
> vfs_streams_xattr and vfs_fruit. vfs_streams_depot doesn't need changes, as it
> works correctly with the vfs_default implementation of async pread/pwrite as it
> provides usable fds.
>
> Btw, with these changes in place, the only thing missing for allowing read/write aio
> on named streams is flush_send/recv. Or was there any other reason for not
> allowing aio on named streams?
>
> Fwiw, a Windows server supports copy-chunk of named streams.
>
> Please review & push if ok. Thanks!

The VFS changes themselves look fine:
Reviewed-by: David Disseldorp <[hidden email]>

Given that named stream copy-chunk requests aren't macOS specific,
please add the generic tests to the smb2.ioctl suite - Is there any
reason why the suite doesn't currently run against the vfs_fruit
shares?

Cheers, David

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix the new async copy-chunk for streams

Samba - samba-technical mailing list
On Sun, May 14, 2017 at 08:25:44PM +0200, David Disseldorp via samba-technical wrote:
> > Please review & push if ok. Thanks!
>
> The VFS changes themselves look fine:
> Reviewed-by: David Disseldorp <[hidden email]>

thanks!

> Given that named stream copy-chunk requests aren't macOS specific,

not from a protocol perspective, but once we go into the VFS stack the streams
stuff is very VFS specific. vfs_fruit implements special handling for two
specific streams and it's those two cases can only be tested in the vfs.fruit
testsuite.

In the testcases I added I test with three different streams: one is just some
arbitrary named stream where none of our VFS modules is expected to do any
special handling, the other two are the two subtle macOS streams, so

file:foo
file:AFP_AfpInfo
file:AFP_Resource

Generally these days the vfs.fruit tests test all relevant permutations of
streams modules (xattr vs depot), with and without vfs_fruit intercepting the
macOS streams. So by putting the testcases in vfs.fruit we effectively test all
cases.

Nevertheless I agree that it makes sense to add a basic test to smb2.ioctl
itself. I will post an updated patchset.

> please add the generic tests to the smb2.ioctl suite - Is there any
> reason why the suite doesn't currently run against the vfs_fruit
> shares?

That wouldn't increase test coverage because smb2.ioctl doesn't use macOS
specific streams.

-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix the new async copy-chunk for streams

Samba - samba-technical mailing list
On Mon, May 15, 2017 at 08:29:00PM +0200, Ralph Böhme via samba-technical wrote:
> Nevertheless I agree that it makes sense to add a basic test to smb2.ioctl
> itself. I will post an updated patchset.

attached.

Fwiw, I tried to consolidate test_setup_copy_chunk() and make it a public
utility function but I failed miserably to figure out the required headers and
deps.

-slow

bug12787-master-v2.patch (43K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix the new async copy-chunk for streams

Samba - samba-technical mailing list
On Tue, 16 May 2017 13:37:46 +0200, Ralph Böhme wrote:

> On Mon, May 15, 2017 at 08:29:00PM +0200, Ralph Böhme via samba-technical wrote:
> > Nevertheless I agree that it makes sense to add a basic test to smb2.ioctl
> > itself. I will post an updated patchset.  
>
> attached.

Thanks, I should get to this tonight or tomorrow.

> Fwiw, I tried to consolidate test_setup_copy_chunk() and make it a public
> utility function but I failed miserably to figure out the required headers and
> deps.

Cool, yeah that's been on my todo list for a while too -
write_pattern() and check_pattern() would be ideal candidates for
consolidation. It can wait for now though.

Cheers, David

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix the new async copy-chunk for streams

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 16 May 2017 13:37:46 +0200, Ralph Böhme via samba-technical wrote:

> On Mon, May 15, 2017 at 08:29:00PM +0200, Ralph Böhme via samba-technical wrote:
> > Nevertheless I agree that it makes sense to add a basic test to smb2.ioctl
> > itself. I will post an updated patchset.  
>
> attached.

Reviewed-by: David Disseldorp <[hidden email]>

Please push along with the attached patch, which is now needed for
selftest when the smb2.ioctl fs_specific suite is run atop Btrfs.

Also, feel free to change the test name to copy_chunk_streams before
pushing if you like.

Cheers, David

0001-selftest-enable-alternate-streams-for-fs_specific-sh.patch (902 bytes) Download Attachment
Loading...