[PATCH] Add missing async fsync_send()/recv() code to ceph VFS.

[Jeremy Allison via samba-technical]

Hi all,

I'm working on simplifying the VFS by removing the synchronous
versions of calls we already have async versions for. Right
now we have fsync_send()/recv(), pwrite_send()/recv() and
pread_send()/recv() - as WELL as fsync, pwrite and pread.

However we still use the sync versions in one or two small
and irritating places, meaning VFS implementors have to
add both sync and async versions of these backend calls
in order to get a fully functioning VFS.

Whilst working on removing the synchronous fsync() call
I discovered that ceph as a backend filesystem only
implements the synchronous fsync backend VFS call, not
the async versions.

This means SMB2 gated onto a ceph backend isn't syncing
files properly when the client requests it.

I created bug: https://bugzilla.samba.org/show_bug.cgi?id=13412
for this.

Patch that implements fsync_send()/recv() by cheating
and using ceph_fsync() synchronously under the covers
attached.

Please review and push if happy ! I need this as a
prerequisite for my other work (ceph is also missing
async pread/pwrite but that's a patch for another day :-).

Cheers,

Jeremy.

[Ralph Böhme via samba-technical]

Hi Jeremy,

Post by Jeremy Allison via samba-technical
Patch that implements fsync_send()/recv() by cheating
and using ceph_fsync() synchronously under the covers
attached.
Please review and push if happy ! I need this as a
prerequisite for my other work (ceph is also missing
async pread/pwrite but that's a patch for another day :-).
diff --git a/source3/modules/vfs_ceph.c b/source3/modules/vfs_ceph.c
index d61213110e0..cbe5fac1f89 100644
--- a/source3/modules/vfs_ceph.c
+++ b/source3/modules/vfs_ceph.c
@@ -569,6 +569,62 @@ static int cephwrap_fsync(struct vfs_handle_struct *handle, files_struct *fsp)
WRAP_RETURN(result);
}
+/*
+ * Fake up an async ceph fsync by calling the sychronous API.
+ */
+
+static struct tevent_req *cephwrap_fsync_send(struct vfs_handle_struct *handle,
+ TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ files_struct *fsp)
+{
+ struct tevent_req *req = NULL;
+ struct vfs_aio_state *state = NULL;
+ struct timespec start_time = { 0 };
+ struct timespec end_time = { 0 };
+ int ret = -1;
+
+ DBG_DEBUG("[CEPH] cephwrap_fsync_send\n");
+
+ req = tevent_req_create(mem_ctx, &state, struct vfs_aio_state);
+ if (req == NULL) {
+ return NULL;
+ }
+
+ PROFILE_TIMESTAMP(&start_time);
+
+ /* Make sync call. */
+ ret = ceph_fsync(handle->data, fsp->fh->fd, false);
+
+ if (ret != 0) {
+ /* ceph_fsync returns -errno on error. */
+ state->error = -ret;
+ }

Shouldn't this be:

if (tevent_req_error(req, ret)) {
return tevent_req_post(req, ev);
}

?

Post by Jeremy Allison via samba-technical
+
+ PROFILE_TIMESTAMP(&end_time);
+ state->duration = nsec_time_diff(&end_time, &start_time);
+
+ /* Mark it as done. */
+ tevent_req_done(req);
+ /* Return and schedule the completion of the call. */
+ return tevent_req_post(req, ev);
+}
+
+static int cephwrap_fsync_recv(struct tevent_req *req,
+ struct vfs_aio_state *vfs_aio_state)
+{
+ struct vfs_aio_state *state =
+ tevent_req_data(req, struct vfs_aio_state);
+
+ DBG_DEBUG("[CEPH] cephwrap_fsync_recv\n");
+
+ *vfs_aio_state = *state;
+ if (state->error != 0) {
+ return -1;
+ }

Then this should be:

if (tevent_req_is_unix_error(req, &vfs_aio_state->error)) {
return -1;
}
*vfs_aio_state = *state;

Post by Jeremy Allison via samba-technical
+ return 0;
+}
+
#ifdef HAVE_CEPH_STATX
#define SAMBA_STATX_ATTR_MASK (CEPH_STATX_BASIC_STATS|CEPH_STATX_BTIME)
@@ -1440,6 +1496,8 @@ static struct vfs_fn_pointers ceph_fns = {
.recvfile_fn = cephwrap_recvfile,
.rename_fn = cephwrap_rename,
.fsync_fn = cephwrap_fsync,
+ .fsync_send_fn = cephwrap_fsync_send,
+ .fsync_recv_fn = cephwrap_fsync_recv,

This uses spaces for indentation.

Otherwise looks good.

-slow

[David Disseldorp via samba-technical]

Hi Ralph and Jeremy,

Post by Ralph Böhme via samba-technical
Hi Jeremy,

Post by Jeremy Allison via samba-technical
Patch that implements fsync_send()/recv() by cheating
and using ceph_fsync() synchronously under the covers
attached.

...

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical
+ PROFILE_TIMESTAMP(&start_time);
+
+ /* Make sync call. */
+ ret = ceph_fsync(handle->data, fsp->fh->fd, false);
+
+ if (ret != 0) {
+ /* ceph_fsync returns -errno on error. */
+ state->error = -ret;
+ }

if (tevent_req_error(req, ret)) {
return tevent_req_post(req, ev);
}
?

We still need to retain the errno negation, so that'd be
tevent_req_error(req, -ret)...

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical
+
+ PROFILE_TIMESTAMP(&end_time);
+ state->duration = nsec_time_diff(&end_time, &start_time);
+
+ /* Mark it as done. */
+ tevent_req_done(req);
+ /* Return and schedule the completion of the call. */
+ return tevent_req_post(req, ev);
+}
+
+static int cephwrap_fsync_recv(struct tevent_req *req,
+ struct vfs_aio_state *vfs_aio_state)
+{
+ struct vfs_aio_state *state =
+ tevent_req_data(req, struct vfs_aio_state);
+
+ DBG_DEBUG("[CEPH] cephwrap_fsync_recv\n");
+
+ *vfs_aio_state = *state;
+ if (state->error != 0) {
+ return -1;
+ }

if (tevent_req_is_unix_error(req, &vfs_aio_state->error)) {
return -1;

This would mean that we don't propagate up the call duration on failure.
I'm not so familiar with the async VFS API, but is that to be expected?

Cheers, David

[Ralph Böhme via samba-technical]

Hi David,

Post by David Disseldorp via samba-technical

Post by Ralph Böhme via samba-technical
Hi Jeremy,

Post by Jeremy Allison via samba-technical
Patch that implements fsync_send()/recv() by cheating
and using ceph_fsync() synchronously under the covers
attached.

...

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical
+ PROFILE_TIMESTAMP(&start_time);
+
+ /* Make sync call. */
+ ret = ceph_fsync(handle->data, fsp->fh->fd, false);
+
+ if (ret != 0) {
+ /* ceph_fsync returns -errno on error. */
+ state->error = -ret;
+ }

if (tevent_req_error(req, ret)) {
return tevent_req_post(req, ev);
}
?

We still need to retain the errno negation, so that'd be
tevent_req_error(req, -ret)...

yep, but the -ret in tevent_req_error() looks somewhat strange, maybe this is
better:

if (ret != 0) {
/* ceph_fsync returns -errno on error. */
tevent_req_error(req, -ret);
return tevent_req_post(req, ev);
}

Post by David Disseldorp via samba-technical

Post by Ralph Böhme via samba-technical
if (tevent_req_is_unix_error(req, &vfs_aio_state->error)) {
return -1;

This would mean that we don't propagate up the call duration on failure.
I'm not so familiar with the async VFS API, but is that to be expected?

vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.

So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

-slow

[Jeremy Allison via samba-technical]

Post by Ralph Böhme via samba-technical
vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.
So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

OK, here is version #2, including the changes you guys discussed.

Please review and push if happy. I have a follow-up patch that
removes all uses of the synchronous fsync_fn() from the VFS once
this one goes in :-).

Thanks !

Jeremy.

[Ralph Böhme via samba-technical]

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical
vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.
So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

OK, here is version #2, including the changes you guys discussed.
Please review and push if happy. I have a follow-up patch that
removes all uses of the synchronous fsync_fn() from the VFS once
this one goes in :-).

pushed.

-slow

[Jeremy Allison via samba-technical]

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical
vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.
So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

OK, here is version #2, including the changes you guys discussed.
Please review and push if happy. I have a follow-up patch that
removes all uses of the synchronous fsync_fn() from the VFS once
this one goes in :-).

pushed.

Hurrah - it went in, thanks ! Now I have your attention,
here is the follow-up that removes the synchronous fsync_fn()
completely from the VFS.

Please review and let me know what you think !

FYI: I'm half way though doing the same for SMB_VFS_READ
(converting that to SMB_VFS_PREAD everywhere and then
moving SMB_VFS_PREAD -> SMB_VFS_PREAD_SEND/SMB_VFS_PREAD_RECV
pairs). Finally I'll get to SMB_VFS_WRITE and do the same,
and we should end up with only *one* way to do a VFS
FSYNC/READ/WRITE in the backend - and that will be asynchronous,
instead of the current two or three different ways, some
sync, some async.

Cheers,

Jeremy.

[Jeremy Allison via samba-technical]

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical
vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.
So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

OK, here is version #2, including the changes you guys discussed.
Please review and push if happy. I have a follow-up patch that
removes all uses of the synchronous fsync_fn() from the VFS once
this one goes in :-).

pushed.

Hurrah - it went in, thanks ! Now I have your attention,
here is the follow-up that removes the synchronous fsync_fn()
completely from the VFS.
Please review and let me know what you think !
FYI: I'm half way though doing the same for SMB_VFS_READ
(converting that to SMB_VFS_PREAD everywhere and then
moving SMB_VFS_PREAD -> SMB_VFS_PREAD_SEND/SMB_VFS_PREAD_RECV
pairs). Finally I'll get to SMB_VFS_WRITE and do the same,
and we should end up with only *one* way to do a VFS
FSYNC/READ/WRITE in the backend - and that will be asynchronous,
instead of the current two or three different ways, some
sync, some async.

FYI Ralph, I know you're busy - but I've now got working
code that builds on top of this patch that eliminates
all uses of SMB_VFS_READ ! SMB_VFS_WRITE next, then
convert all users of SMB_VFS_PREAD/SMB_VFS_PWRITE to
the async versions, then we have the holy grail of
all read/write I/O being done async as the only method
available !

Jeremy.

[Jeremy Allison via samba-technical]

Post by Jeremy Allison via samba-technical

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical
vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.
So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

OK, here is version #2, including the changes you guys discussed.
Please review and push if happy. I have a follow-up patch that
removes all uses of the synchronous fsync_fn() from the VFS once
this one goes in :-).

pushed.

Hurrah - it went in, thanks ! Now I have your attention,
here is the follow-up that removes the synchronous fsync_fn()
completely from the VFS.
Please review and let me know what you think !
FYI: I'm half way though doing the same for SMB_VFS_READ
(converting that to SMB_VFS_PREAD everywhere and then
moving SMB_VFS_PREAD -> SMB_VFS_PREAD_SEND/SMB_VFS_PREAD_RECV
pairs). Finally I'll get to SMB_VFS_WRITE and do the same,
and we should end up with only *one* way to do a VFS
FSYNC/READ/WRITE in the backend - and that will be asynchronous,
instead of the current two or three different ways, some
sync, some async.

FYI Ralph, I know you're busy - but I've now got working
code that builds on top of this patch that eliminates
all uses of SMB_VFS_READ ! SMB_VFS_WRITE next, then
convert all users of SMB_VFS_PREAD/SMB_VFS_PWRITE to
the async versions, then we have the holy grail of
all read/write I/O being done async as the only method
available !

Once the fsync changes have gone in I'll start
sending this through in smaller digestible
chunks. The only larger changes are (would you
belive it :-) in source3/printing/nt_printing.c
which is an old, heavy user of the LSEEK/READ
style interface.

Jeremy.

[Uri Simchoni via samba-technical]

Post by Jeremy Allison via samba-technical

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical

Post by Ralph Böhme via samba-technical
vfs_ceph doesn't implement profiling so it doesn't really matter, cf the
SMBPROFILE_BYTES_ASYNC_* macros in vfs_default how it would look like.
So I guess you can either remove the PROFILE_TIMESTAMP macro invocations or
implement the profiling stuff.

OK, here is version #2, including the changes you guys discussed.
Please review and push if happy. I have a follow-up patch that
removes all uses of the synchronous fsync_fn() from the VFS once
this one goes in :-).

pushed.

Hurrah - it went in, thanks ! Now I have your attention,
here is the follow-up that removes the synchronous fsync_fn()
completely from the VFS.
Please review and let me know what you think !
FYI: I'm half way though doing the same for SMB_VFS_READ
(converting that to SMB_VFS_PREAD everywhere and then
moving SMB_VFS_PREAD -> SMB_VFS_PREAD_SEND/SMB_VFS_PREAD_RECV
pairs). Finally I'll get to SMB_VFS_WRITE and do the same,
and we should end up with only *one* way to do a VFS
FSYNC/READ/WRITE in the backend - and that will be asynchronous,
instead of the current two or three different ways, some
sync, some async.

FYI Ralph, I know you're busy - but I've now got working
code that builds on top of this patch that eliminates
all uses of SMB_VFS_READ ! SMB_VFS_WRITE next, then
convert all users of SMB_VFS_PREAD/SMB_VFS_PWRITE to
the async versions, then we have the holy grail of
all read/write I/O being done async as the only method
available !
Jeremy.

Hmm... if you can get rid of READ, that's great but do you really want
to get rid of PREAD/PWRITE?

To quote your SambaXP performance talk [1], the best IO strategy
"Completely depends on available system resources" and on access patterns.

Taking the "tweaking power" away from users/vendors might upset some people.

[1]
https://sambaxp.org/archive_data/SambaXP2012-DATA/thu/track1/Jeremy_Allison-The-Evolution-of-IO.pdf


Thanks,
Uri.

[Jeremy Allison via samba-technical]

Post by Uri Simchoni via samba-technical
Hmm... if you can get rid of READ, that's great but do you really want
to get rid of PREAD/PWRITE?
To quote your SambaXP performance talk [1], the best IO strategy
"Completely depends on available system resources" and on access patterns.
Taking the "tweaking power" away from users/vendors might upset some people.
[1]
https://sambaxp.org/archive_data/SambaXP2012-DATA/thu/track1/Jeremy_Allison-The-Evolution-of-IO.pdf

Well, I'm trying to simplify the VFS so that the
primary access methods are async.

If you want sync you can always wrap the sync calls
inside an async tevent wrapper - exactly how I did
in the "fake" async fsync_send/fsync_recv I implemented
for ceph (as far as I know, the ceph userspace library
doesn't have async IO). That way, although the smbd
code is making async calls, underneath the covers
it's actually doing sync requests.

It does make creating a VFS a little more complex,
in that you need to understand tevent in order to
implement, but that requirement is coming anyway as
metze and Ralph are planning on moving more of our
underlying file accesses to the
async-with-pthread-credentials model.

Right now a read request does:

a). Attempt pread_send().
b). If that fails (not implemented) try sendfile
c). If that isn't present fall back to synchronous
pread.

Write requests do the same, only using recvfile
instead of sendfile.

The only problem I can see with hiding the
sync pread inside an async pread_send interface
is that it breaks the ability to fall back to
sendfile if there really isn't an async pread
available.

Now would be a good time for vendors with exotic
filesystem backends to chime in if it's really
easier for us to keep the two pread/pread_send
read methods (I don't think there's any argument
that simple 'read' needs to go :-).

Jeremy.

[Uri Simchoni via samba-technical]

Post by Jeremy Allison via samba-technical

Post by Uri Simchoni via samba-technical
Hmm... if you can get rid of READ, that's great but do you really want
to get rid of PREAD/PWRITE?
To quote your SambaXP performance talk [1], the best IO strategy
"Completely depends on available system resources" and on access patterns.
Taking the "tweaking power" away from users/vendors might upset some people.
[1]
https://sambaxp.org/archive_data/SambaXP2012-DATA/thu/track1/Jeremy_Allison-The-Evolution-of-IO.pdf

Well, I'm trying to simplify the VFS so that the
primary access methods are async.
If you want sync you can always wrap the sync calls
inside an async tevent wrapper - exactly how I did
in the "fake" async fsync_send/fsync_recv I implemented
for ceph (as far as I know, the ceph userspace library
doesn't have async IO). That way, although the smbd
code is making async calls, underneath the covers
it's actually doing sync requests.

Fair enough - so we can keep "aio write size" and just tuck it into the
default VFS, or remove it and then restore it into the default VFS if
too many people complain :)

[Jeremy Allison via samba-technical]

Post by Uri Simchoni via samba-technical

Post by Jeremy Allison via samba-technical

Post by Uri Simchoni via samba-technical
Hmm... if you can get rid of READ, that's great but do you really want
to get rid of PREAD/PWRITE?
To quote your SambaXP performance talk [1], the best IO strategy
"Completely depends on available system resources" and on access patterns.
Taking the "tweaking power" away from users/vendors might upset some people.
[1]
https://sambaxp.org/archive_data/SambaXP2012-DATA/thu/track1/Jeremy_Allison-The-Evolution-of-IO.pdf

Well, I'm trying to simplify the VFS so that the
primary access methods are async.
If you want sync you can always wrap the sync calls
inside an async tevent wrapper - exactly how I did
in the "fake" async fsync_send/fsync_recv I implemented
for ceph (as far as I know, the ceph userspace library
doesn't have async IO). That way, although the smbd
code is making async calls, underneath the covers
it's actually doing sync requests.

Fair enough - so we can keep "aio write size" and just tuck it into the
default VFS, or remove it and then restore it into the default VFS if
too many people complain :)

I was thinking more... In order to keep the ability
to have:

try real async
-> fallback: try recvfile
-> fallback: try sync

We could add an bool to the _send VFS calls that
we set to true when we insist on "true" async,
and set to false when we can code with 'faked'
async (sync using tevent under the covers).

Argghh. This is getting too complex. Maybe
it's just better to keep the PREAD/PWRITE
VFS calls as sync versions separate instead.

[Ralph Böhme via samba-technical]

Post by Jeremy Allison via samba-technical
I was thinking more... In order to keep the ability
try real async
-> fallback: try recvfile
-> fallback: try sync
We could add an bool to the _send VFS calls that
we set to true when we insist on "true" async,
and set to false when we can code with 'faked'
async (sync using tevent under the covers).
Argghh. This is getting too complex. Maybe
it's just better to keep the PREAD/PWRITE
VFS calls as sync versions separate instead.

hm, it seems the user-facing order of aio vs sendfile is not specified, at least
not in smb.conf, is it? Can we get away with just switching the order of
preference? Ie if sendfile is enabled, just use sendfile, if not, use aio with a
possible sync io fallback under the covers.

-slow

[Jeremy Allison via samba-technical]

Post by Ralph Böhme via samba-technical

Post by Jeremy Allison via samba-technical
I was thinking more... In order to keep the ability
try real async
-> fallback: try recvfile
-> fallback: try sync
We could add an bool to the _send VFS calls that
we set to true when we insist on "true" async,
and set to false when we can code with 'faked'
async (sync using tevent under the covers).
Argghh. This is getting too complex. Maybe
it's just better to keep the PREAD/PWRITE
VFS calls as sync versions separate instead.

hm, it seems the user-facing order of aio vs sendfile is not specified, at least
not in smb.conf, is it? Can we get away with just switching the order of
preference? Ie if sendfile is enabled, just use sendfile, if not, use aio with a
possible sync io fallback under the covers.

Yes, that would work. We could only do this on a 4.9.x
release though, as it will change the behavior of
existing smb.conf's that have both aio and sendfile
specified.

But that's true for these changes generally as they
are VFS ABI changes, so that is indeed a solution.

I have the patches ready that remove SMB_VFS_READ,
let me do write next and then let's make a final
decision on SMB_VFS_PREAD/SMB_VFS_PWRITE.

Jeremy.

[Ralph Böhme via samba-technical]

Post by Jeremy Allison via samba-technical
FYI Ralph, I know you're busy - but I've now got working
code that builds on top of this patch that eliminates
all uses of SMB_VFS_READ !

Nah, today is just public holiday. :)

-slow

[Ralph Böhme via samba-technical]

Post by Jeremy Allison via samba-technical
Hurrah - it went in, thanks ! Now I have your attention,
here is the follow-up that removes the synchronous fsync_fn()
completely from the VFS.
Please review and let me know what you think !

lgtm, pushed. Thanks!

Post by Jeremy Allison via samba-technical
FYI: I'm half way though doing the same for SMB_VFS_READ
(converting that to SMB_VFS_PREAD everywhere and then
moving SMB_VFS_PREAD -> SMB_VFS_PREAD_SEND/SMB_VFS_PREAD_RECV
pairs). Finally I'll get to SMB_VFS_WRITE and do the same,
and we should end up with only *one* way to do a VFS
FSYNC/READ/WRITE in the backend - and that will be asynchronous,
instead of the current two or three different ways, some
sync, some async.

awesome!

-slow