[PATCHES] Issue fsync for SMB2 FLUSH asynchronously
Jeremy Allison
jra at samba.org
Wed Nov 11 23:21:13 UTC 2015
On Wed, Nov 11, 2015 at 02:37:06PM -0800, Jeremy Allison wrote:
> On Wed, Nov 11, 2015 at 03:32:59PM -0700, Christof Schmitt wrote:
> > On Wed, Nov 11, 2015 at 01:27:50PM -0800, Jeremy Allison wrote:
> > > On Wed, Nov 11, 2015 at 01:36:32PM -0700, Christof Schmitt wrote:
> > > >
> > > > From 4762b67740770f1ba43e78349c8d1f48857b55dd Mon Sep 17 00:00:00 2001
> > > > From: Christof Schmitt <cs at samba.org>
> > > > Date: Wed, 11 Nov 2015 13:31:15 -0700
> > > > Subject: [PATCH 2/2] smbd: Issue fsync for SMB2 FLUSH asynchronously
> > > >
> > > > SMB2 FLUSH mainly calls fsync and there is already code in place to
> > > > handle fsync asynchronously, so use the asynchronous code path for SMB2
> > > > FLUSH. This avoids a SMB2 FLUSH stalling other requests processing.
> > >
> > > This code isn't quite right I'm afraid.
> > >
> > > It would end up calling tevent_req_done()
> > > twice, once in at the end of smbd_smb2_flush_send()
> > > and then again in smbd_smb2_flush_done() when
> > > when the subreq completes.
> >
> > Yes, sorry. I meant to handle this correctly, but then forgot again.
> >
> > >
> > > The correct thing to do in the:
> > >
> > > if (lp_strict_sync(SNUM(smbreq->conn))) {
> > >
> > > block is to add a cancellation function
> > > to the outstanding req using tevent_req_set_cancel_fn()
> > > (see smbd_smb2_write_send() for details) to allow
> > > the client to send a cancel on this outstanding request,
> > > then add the req to the pending aio queue
> > > using aio_add_req_to_fsp() (see smbd_smb2_setinfo_send()
> > > for details) to ensure that if the file gets
> > > closed whilst this flush request is outstanding
> > > then when the flush completes it doesn't
> > > crash smbd.
> > >
> > > I'll code this up for you if you like as
> > > it is a little trickier than it looks.
> >
> > Looking at smbd_smb2_write_cancel this does not seem to be too
> > difficult. Let me try to add this.
>
> Errr. Sorry, I already did it :-(.
>
> > > Thanks a *LOT* for doing this though !
> > > It's something we've needed for a while.
> >
> > Agreed, it is needed. When looking through the code, i started wondering
> > why this has not been done before, given that there is already an async
> > FSYNC available.
>
> Turns out we don't need the cancellation
> function (not really cancellable).
>
> Here is the version I have and am
> testing right now.
>
> Let me know if this works for you.
Hmmm. In test I'm getting
[180(681)/1881 at 31m13s] samba3.smbtorture_s3.plain(nt4_dc).SMB2-BASIC
UNEXPECTED(failure): samba3.smbtorture_s3.plain(nt4_dc).SMB2-BASIC.smbtorture(nt4_dc)
REASON: Exception: Exception: using seed 1447283296
host=127.0.0.3 share=tmp user=jra myname=localnt4dc2
Running SMB2-BASIC
Starting SMB2-BASIC
smb2cli_flush returned NT_STATUS_IO_TIMEOUT
TEST SMB2-BASIC FAILED!
Let me look at this some more..
> From 01ad975c265982dc2ae04f4207e849587c883409 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 11 Nov 2015 13:28:09 -0700
> Subject: [PATCH 1/2] selftest: Use strict sync = yes
>
> This enables the codepaths calling fsync for FLUSH requests during
> selftest.
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> Reviewed-by: Jeremy Allison <jra at samba.org>
> ---
> selftest/target/Samba3.pm | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 4b7882b..1c54dae 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -1331,6 +1331,7 @@ sub provision($$$$$$$$)
> create mask = 755
> dos filemode = yes
> strict rename = yes
> + strict sync = yes
> vfs objects = acl_xattr fake_acls xattr_tdb streams_depot
>
> printing = vlp
> --
> 2.6.0.rc2.230.g3dd15c0
>
>
> From 11d01eaab76dd05cb4c1c7e02bacfcf354da9a9e Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 11 Nov 2015 13:31:15 -0700
> Subject: [PATCH 2/2] smbd: Issue fsync for SMB2 FLUSH asynchronously
>
> SMB2 FLUSH mainly calls fsync and there is already code in place to
> handle fsync asynchronously, so use the asynchronous code path for SMB2
> FLUSH. This avoids a SMB2 FLUSH stalling other requests processing.
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/smb2_flush.c | 57 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c
> index 04a8710..15dc538 100644
> --- a/source3/smbd/smb2_flush.c
> +++ b/source3/smbd/smb2_flush.c
> @@ -110,15 +110,18 @@ struct smbd_smb2_flush_state {
> struct smbd_smb2_request *smb2req;
> };
>
> +static void smbd_smb2_flush_done(struct tevent_req *subreq);
> +
> static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct smbd_smb2_request *smb2req,
> struct files_struct *fsp)
> {
> struct tevent_req *req;
> + struct tevent_req *subreq;
> struct smbd_smb2_flush_state *state;
> - NTSTATUS status;
> struct smb_request *smbreq;
> + int ret;
>
> req = tevent_req_create(mem_ctx, &state,
> struct smbd_smb2_flush_state);
> @@ -145,16 +148,56 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx,
> return tevent_req_post(req, ev);
> }
>
> - status = sync_file(smbreq->conn, fsp, true);
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(5,("smbd_smb2_flush: sync_file for %s returned %s\n",
> - fsp_str_dbg(fsp), nt_errstr(status)));
> - tevent_req_nterror(req, status);
> + if (fsp->fh->fd == -1) {
> + tevent_req_nterror(req, NT_STATUS_INVALID_HANDLE);
> + return tevent_req_post(req, ev);
> + }
> +
> + if (!lp_strict_sync(SNUM(smbreq->conn))) {
> + /*
> + * No strict sync. Don't really do
> + * anything here.
> + */
> + tevent_req_done(req);
> + return tevent_req_post(req, ev);
> + }
> +
> + ret = flush_write_cache(fsp, SAMBA_SYNC_FLUSH);
> + if (ret == -1) {
> + tevent_req_nterror(req, map_nt_error_from_unix(errno));
> + return tevent_req_post(req, ev);
> + }
> +
> + subreq = SMB_VFS_FSYNC_SEND(state, ev, fsp);
> + if (tevent_req_nomem(subreq, req)) {
> return tevent_req_post(req, ev);
> }
>
> + tevent_req_set_callback(subreq, smbd_smb2_flush_done, req);
> +
> + /* Ensure any close request knows about this outstanding IO. */
> + if (!aio_add_req_to_fsp(fsp, req)) {
> + tevent_req_nterror(req, NT_STATUS_NO_MEMORY);
> + return tevent_req_post(req, ev);
> + }
> +
> + return req;
> +
> +}
> +
> +static void smbd_smb2_flush_done(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + int ret, err;
> +
> + ret = SMB_VFS_FSYNC_RECV(subreq, &err);
> + TALLOC_FREE(subreq);
> + if (ret == -1) {
> + tevent_req_error(req, err);
> + return;
> + }
> tevent_req_done(req);
> - return tevent_req_post(req, ev);
> }
>
> static NTSTATUS smbd_smb2_flush_recv(struct tevent_req *req)
> --
> 2.6.0.rc2.230.g3dd15c0
>
More information about the samba-technical
mailing list