Re: [RFC PATCH 2/4] ksmbd: route WRITE requests to an unbound workqueue

[Namjae Jeon]

> @@ -17,6 +18,23 @@
>  static struct kmem_cache *work_cache;
>  static struct workqueue_struct *ksmbd_wq;
>
> +/* Load-time switch for the WRITE request routing policy. */
> +bool ksmbd_write_wq_enabled;
> +module_param_named(write_wq, ksmbd_write_wq_enabled, bool, 0444);
> +MODULE_PARM_DESC(write_wq,
> +                "Route eligible WRITEs to an unbound workqueue; load-time only");
> +
Since this improves performance, why isn't it enabled by default?
Also, I would like this setting to be on/off via ksmbd.conf.

> +enum ksmbd_wq_class {
> +       KSMBD_WQ_LOCAL,
> +       KSMBD_WQ_WRITE,
> +       KSMBD_WQ_BLOCK_WRITE,
> +};
> +
> +/*
> + * Classify a complete RFC1002-framed request for workqueue placement only.
> + * Check bounds before inspecting fields; leave protocol validation to the worker.
> + */
> +static enum ksmbd_wq_class ksmbd_smb2_wq_classify(void *buf)
> +{
> +       const struct smb2_hdr *hdr = smb_get_msg(buf);
> +       const struct smb2_write_req *req = (const void *)hdr;
> +       unsigned int len = get_rfc1002_len(buf);
> +
> +       if (len < sizeof(*hdr) || hdr->ProtocolId != SMB2_PROTO_NUMBER ||
> +           hdr->NextCommand ||
> +           (hdr->Flags & (SMB2_FLAGS_SERVER_TO_REDIR | SMB2_FLAGS_ASYNC_COMMAND |
> +                          SMB2_FLAGS_RELATED_OPERATIONS | SMB2_FLAGS_SIGNED)))
> +               return KSMBD_WQ_BLOCK_WRITE;
> +
> +       switch (get_unaligned_le16(&hdr->Command)) {
> +       case SMB2_WRITE_HE:
> +               if (len < sizeof(*req) || !req->Length ||
> +                   req->Channel != SMB2_CHANNEL_NONE || req->Flags)
> +                       return KSMBD_WQ_BLOCK_WRITE;
For 4 KiB single-channel WRITEs, throughput drops from 219.55 to
193.01 MiB/s. Should we add a size check here to keep small WRITEs on
the per-CPU queue?

> +               return KSMBD_WQ_WRITE;
> +       case SMB2_NEGOTIATE_HE:
> +       case SMB2_SESSION_SETUP_HE:
> +       case SMB2_LOGOFF_HE:
> +       case SMB2_TREE_CONNECT_HE:
> +       case SMB2_TREE_DISCONNECT_HE:
> +       case SMB2_CREATE_HE:
> +       case SMB2_CLOSE_HE:
> +       case SMB2_FLUSH_HE:
> +       case SMB2_CANCEL_HE:
> +       case SMB2_ECHO_HE:
> +       case SMB2_QUERY_INFO_HE:
> +       case SMB2_SET_INFO_HE:
> +               /* Keep these commands per-CPU without blocking the WRITE queue. */
> +               return KSMBD_WQ_LOCAL;
> +       default:
> +               /* READ and unclassified traffic also keep later WRITEs per-CPU. */
> +               return KSMBD_WQ_BLOCK_WRITE;
> +       }
> +}
> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> index b7ce67094626..54ecdd096bd2 100644
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -125,6 +125,12 @@ static int register_session_channel(struct ksmbd_session *sess,
>         if (xa_is_err(old)) {
>                 kfree_sensitive(chann);
>                 rc = xa_err(old);
> +               goto out;
> +       }
> +       if (ksmbd_write_wq_enabled && count) {
> +               /* Remember the extra channel even if binding later fails. */
> +               xa_for_each(&sess->ksmbd_chann_list, index, chann)
> +                       WRITE_ONCE(chann->conn->multichannel_seen, true);
This walks the entire channel list and sets multichannel_seen again
whenever an additional channel is registered. Since the flag is
sticky, all existing channels are already marked when count > 1. Could
we walk the list only when count == 1, marking both the existing and
newly added connections, and update only conn->multichannel_seen when
count > 1? This would avoid the repeated traversal and stores.