пн, 9 сент. 2019 г. в 07:21, Steve French <smfrench@xxxxxxxxx>:
>
> Had a minor typo in patch 2 - fixed in attached
>
> On Sun, Sep 8, 2019 at 11:31 PM Steve French <smfrench@xxxxxxxxx> wrote:
> >
> > I am seeing very good performance benefit from offload of decryption
This is great news!
> > of encrypted SMB3 read responses to a pool of worker threads
> > (optionally). See attached patches.
> >
> > I plan to add another patch to only offload when number of requests in
> > flight is > 1 (since there is no point offloading and doing a thread
> > switch if no other responses would overlap in the cifsd thread reading
> > from the socket).
Good idea. The 2nd path looks good. See feedback from the 1st patch below:
+ mid = smb2_find_mid(dw->server, dw->buf);
+ if (mid == NULL)
+ cifs_dbg(VFS, "mid not found\n"); /* BB FIXME? change to FYI? */
Yes, let's keep it the same - FYI - as the non-offloaded scenario.
---------------------
I think there is a race on mid structure between offloaded work items
and the demultiplex thread.
+ mid = smb2_find_mid(dw->server, dw->buf);
^^^
Here we took a reference to the mid...
+ if (mid == NULL)
+ cifs_dbg(VFS, "mid not found\n"); /* BB FIXME? change to FYI? */
+ else {
+ mid->decrypted = true;
+ rc = handle_read_data(dw->server, mid, dw->buf,
^^^
...and the above call will dequeue the mid from the list. Between this
two steps the demultiplex thread might hit reconnect (cifs_reconnect)
and fire the mid callback.
+ dw->server->vals->read_rsp_size,
+ dw->ppages, dw->npages, dw->len);
+ }
+
+ dw->server->lstrp = jiffies;
The mid callback for async reads will fail the read request and then
call DeleteMidQEntry which resets the mid state to MID_FREE and do
other things we don't want to be done at this point.
So, I think the following needs to be done to avoid it:
1) dequeue the mid inside analog of smb2_find_mid() - let's say
smb2_find_mid_dequeue() and call it instead.
2) refactor handle_read_data() into two parts to separate mid handling
(part 1) and dequeue'ing (part2) - note, that the latter is being
called in non-negative return code cases.
3) call only the part 1 in the offloaded case because the mid has been
already dequeue'ed at the step 1.
+ mid->callback(mid);
+
+ cifs_mid_q_entry_release(mid);
+
+free_pages:
+ for (i = dw->npages-1; i >= 0; i--)
+ put_page(dw->ppages[i]);
+
+ kfree(dw->ppages);
+ cifs_small_buf_release(dw->buf);
+
+/* FIXME Do we need the equivalent of this? */
No, we don't need this because the entire packet has been received by
this point.
+/* discard_data:
+ cifs_discard_remaining_data(server); */
+}
--
Best regards,
Pavel Shilovsky
