Re: [RFC PATCH for-next] smb: client: parallelize multichannel write issue

On 06/26, Henrique Carvalho wrote:
>I'm sending this as an RFC PATCH first so the approach and results can
>be sanity checked more broadly.
>
>The netfs writeback path issues write subrequests through the filesystem
>issue_write() callback. For multichannel, those subrequests may target
>different channels, but the issue callback is still entered serially by
>the netfs issuing context.
>
>As a result, while one channel is running the write issue path, write
>subrequests for other channels may be left waiting to be issued. This
>can limit multichannel writeback throughput because the channels are not
>kept busy independently.
>
>For multichannel sessions, queue the existing write issue path to a
>workqueue. This lets the netfs issuing context return quickly and
>continue issuing subsequent write subrequests for other channels.
>Single-channel sessions keep the existing synchronous issue path.
>
>Preliminary fio testing showed improvments in throughput by up to 2.5x
>in 4MiB writes with larger dirty limits (1g/256m), 1.4x improvement for
>1GiB writes with larger dirty limits, and is neutral when dirty limits
>keep the pipeline shallow (4m/1m).

Works great.

A few concerns for a next version inlined below.

>+	destroy_workqueue(cifs_write_issue_wq);

Make sure to flush the workqueue at the appropriate time for a clean
destroy.

>+static void __cifs_issue_write(struct netfs_io_subrequest *subreq);
>+
>+struct cifs_issue_write_work {
>+	struct work_struct work;
>+	struct netfs_io_subrequest *subreq;
>+};
>+
>+static bool cifs_write_is_mchan(struct cifs_ses *ses)
>+{
>+	bool is_mchan;
>+
>+	spin_lock(&ses->chan_lock);
>+	is_mchan = ses->chan_count > 1;
>+	spin_unlock(&ses->chan_lock);
>+
>+	return is_mchan;
>+}

Not really a concern here, but why limit it to multichannel?
AFAICS single channel mounts would also benefit from this, no?

>+static void cifs_issue_write_work_fn(struct work_struct *work)
>+{
>+	struct cifs_issue_write_work *w = container_of(work, struct cifs_issue_write_work, work);
>+
>+	__cifs_issue_write(w->subreq);
>+	kfree(w);
>+}
>+
>+static int cifs_issue_parallel_write(struct cifs_ses *ses,
>+				      struct netfs_io_subrequest *subreq)
>+{
>+	struct cifs_issue_write_work *w = kmalloc_obj(*w, GFP_NOFS);
>+
>+	if (!w)
>+		return -ENOMEM;
>+
>+	w->subreq = subreq;
>+	INIT_WORK(&w->work, cifs_issue_write_work_fn);
>+	queue_work(cifs_write_issue_wq, &w->work);
>+
>+	return 0;
>+}

I think you need to find a way to track these works somehow, so you can
e.g.:
- capture/propagate -ERESTARTSYS/-EINTR
- (and thus) properly cancel_work() when needed

I tested the patch with 4 channels and thousands of writer processes and
it worked great on a healthy scenario.

Dropping the network mid-operation shows that, after reconnect is
successful, if I kill my writers and try to umount, there are several
cifs_write_issue kworkers hanging (didn't investigate further).

>+static void cifs_issue_write(struct netfs_io_subrequest *subreq)
>+{
>+	struct cifs_io_subrequest *wdata = container_of(subreq, struct cifs_io_subrequest, subreq);
>+	struct cifs_ses *ses = tlink_tcon(wdata->req->cfile->tlink)->ses;
>+
>+	if (cifs_write_is_mchan(ses)) {
>+		int err = cifs_issue_parallel_write(ses, subreq);
>+
>+		if (!err)
>+			return;
>+	}
>+
>+	__cifs_issue_write(subreq);
>+}

cifs_issue_parallel_write() returns -ENOMEM or 0.  If it returns
-ENOMEM you really shouldn't fallback to __cifs_issue_write(), but
rather follow __cifs_issue_write() "fail" case (to make netfs aware
of the error).


Cheers,

Enzo