Apologies, just cleaning out old mail and finding some I should have
responded to long ago:
On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> The workqueue "callback_wq" queues a single work item &cb->cb_work per
> nfsd4_callback instance and thus, it doesn't require execution ordering.
What's "execution ordering"?
We definitely do depend on the fact that at most one of these is running
at a time.
--b.
> Hence, alloc_workqueue has been used to replace the
> deprecated create_singlethread_workqueue instance.
>
> The WQ_MEM_RECLAIM flag has not been set since this is an in-kernel nfs
> server and isn't involved in memory reclaim operations on the local
> host.
>
> Since there are fixed number of work items, explicit concurrency
> limit is unnecessary here.
>
> Signed-off-by: Bhaktipriya Shridhar <[email protected]>
> ---
> Changes in v2:
> - No change. Made this a separate patch (categorised based on
> directories).
>
> fs/nfsd/nfs4callback.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 7389cb1..a6611c6 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -1021,7 +1021,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {
>
> int nfsd4_create_callback_queue(void)
> {
> - callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
> + callback_wq = alloc_workqueue("nfsd4_callbacks", 0, 0);
> if (!callback_wq)
> return -ENOMEM;
> return 0;
> --
> 2.1.4
Hello, Bruce.
On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> Apologies, just cleaning out old mail and finding some I should have
> responded to long ago:
>
> On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> > The workqueue "callback_wq" queues a single work item &cb->cb_work per
> > nfsd4_callback instance and thus, it doesn't require execution ordering.
>
> What's "execution ordering"?
>
> We definitely do depend on the fact that at most one of these is running
> at a time.
If there can be multiple cb's and thus cb->cb_work's per callback_wq,
it'd need explicit ordering. Is that the case?
Thanks.
--
tejun
On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> Hello, Bruce.
>
> On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > Apologies, just cleaning out old mail and finding some I should have
> > responded to long ago:
> >
> > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> > > The workqueue "callback_wq" queues a single work item &cb->cb_work per
> > > nfsd4_callback instance and thus, it doesn't require execution ordering.
> >
> > What's "execution ordering"?
> >
> > We definitely do depend on the fact that at most one of these is running
> > at a time.
>
> If there can be multiple cb's and thus cb->cb_work's per callback_wq,
> it'd need explicit ordering. Is that the case?
Yes, there can be multiple cb_work's.
--b.
On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote:
> On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> >
> > Hello, Bruce.
> >
> > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > >
> > > Apologies, just cleaning out old mail and finding some I should have
> > > responded to long ago:
> > >
> > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> > > >
> > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per
> > > > nfsd4_callback instance and thus, it doesn't require execution ordering.
> > >
> > > What's "execution ordering"?
> > >
AIUI, it means that jobs are always run in the order queued and are
serialized.
> > > We definitely do depend on the fact that at most one of these is running
> > > at a time.
> >
We do?
> > If there can be multiple cb's and thus cb->cb_work's per callback_wq,
> > it'd need explicit ordering. Is that the case?
>
These are basically client RPC tasks, and the cb_work just handles the
submission into the client RPC state machine. Just because we're running
several callbacks at the same time doesn't mean that they need to be
strictly ordered. The client state machine can certainly handle running
these in parallel.
> Yes, there can be multiple cb_work's.
>
Yes, but each is effectively a separate work unit. I see no reason why
we'd need to order them at all.
--
Jeff Layton <[email protected]>
On Wed, 2016-11-09 at 08:18 -0500, Jeff Layton wrote:
> On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote:
> >
> > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> > >
> > >
> > > Hello, Bruce.
> > >
> > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > > >
> > > >
> > > > Apologies, just cleaning out old mail and finding some I should
> > > > have
> > > > responded to long ago:
> > > >
> > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar
> > > > wrote:
> > > > >
> > > > >
> > > > > The workqueue "callback_wq" queues a single work item &cb-
> > > > > >cb_work per
> > > > > nfsd4_callback instance and thus, it doesn't require
> > > > > execution ordering.
> > > >
> > > > What's "execution ordering"?
> > > >
>
> AIUI, it means that jobs are always run in the order queued and are
> serialized.
>
> >
> > >
> > > >
> > > > We definitely do depend on the fact that at most one of these
> > > > is running
> > > > at a time.
> > >
>
> We do?
>
> >
> > >
> > > If there can be multiple cb's and thus cb->cb_work's per
> > > callback_wq,
> > > it'd need explicit ordering. Is that the case?
> >
>
> These are basically client RPC tasks, and the cb_work just handles
> the
> submission into the client RPC state machine. Just because we're
> running
> several callbacks at the same time doesn't mean that they need to be
> strictly ordered. The client state machine can certainly handle
> running
> these in parallel.
>
> >
> > Yes, there can be multiple cb_work's.
> >
>
> Yes, but each is effectively a separate work unit. I see no reason
> why
> we'd need to order them at all.
>
There needs to be serialisation at the session level (i.e. the
callbacks have to respect the slot limits set by the client) however
there shouldn’t be a need for serialisation at the RPC level.
Cheers
Trond
On Wed, 2016-11-09 at 15:08 +0000, Trond Myklebust wrote:
> On Wed, 2016-11-09 at 08:18 -0500, Jeff Layton wrote:
> >
> > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote:
> > >
> > >
> > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> > > >
> > > >
> > > >
> > > > Hello, Bruce.
> > > >
> > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > > > >
> > > > >
> > > > >
> > > > > Apologies, just cleaning out old mail and finding some I should
> > > > > have
> > > > > responded to long ago:
> > > > >
> > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar
> > > > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > The workqueue "callback_wq" queues a single work item &cb-
> > > > > > >
> > > > > > > cb_work per
> > > > > > nfsd4_callback instance and thus, it doesn't require
> > > > > > execution ordering.
> > > > >
> > > > > What's "execution ordering"?
> > > > >
> >
> > AIUI, it means that jobs are always run in the order queued and are
> > serialized.
> >
> > >
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > We definitely do depend on the fact that at most one of these
> > > > > is running
> > > > > at a time.
> > > >
> >
> > We do?
> >
> > >
> > >
> > > >
> > > >
> > > > If there can be multiple cb's and thus cb->cb_work's per
> > > > callback_wq,
> > > > it'd need explicit ordering. Is that the case?
> > >
> >
> > These are basically client RPC tasks, and the cb_work just handles
> > the
> > submission into the client RPC state machine. Just because we're
> > running
> > several callbacks at the same time doesn't mean that they need to be
> > strictly ordered. The client state machine can certainly handle
> > running
> > these in parallel.
> >
> > >
> > >
> > > Yes, there can be multiple cb_work's.
> > >
> >
> > Yes, but each is effectively a separate work unit. I see no reason
> > why
> > we'd need to order them at all.
> >
>
> There needs to be serialisation at the session level (i.e. the
> callbacks have to respect the slot limits set by the client) however
> there shouldn’t be a need for serialisation at the RPC level.
>
> Cheers
> Trond
Yes, that all happens in nfsd4_cb_prepare, which is the rpc_call_prepare
operation for the callback. That gets run by the rpc state machine in
the context of the rpciod workqueues. None of that happens in the
context of the cb_work here.
If you have a look at nfsd4_run_cb_work, you can see that it just does a
cb_ops->prepare and then submits it to the client rpc engine with
rpc_call_async. None of that should require singlethreaded workqueue
semantics.
--
Jeff Layton <[email protected]>
On Wed, Nov 09, 2016 at 08:18:08AM -0500, Jeff Layton wrote:
> On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote:
> > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> > >
> > > Hello, Bruce.
> > >
> > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > > >
> > > > Apologies, just cleaning out old mail and finding some I should have
> > > > responded to long ago:
> > > >
> > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> > > > >
> > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per
> > > > > nfsd4_callback instance and thus, it doesn't require execution ordering.
> > > >
> > > > What's "execution ordering"?
> > > >
>
> AIUI, it means that jobs are always run in the order queued and are
> serialized.
>
> > > > We definitely do depend on the fact that at most one of these is running
> > > > at a time.
> > >
>
> We do?
>
> > > If there can be multiple cb's and thus cb->cb_work's per callback_wq,
> > > it'd need explicit ordering. Is that the case?
> >
>
> These are basically client RPC tasks, and the cb_work just handles the
> submission into the client RPC state machine. Just because we're running
> several callbacks at the same time doesn't mean that they need to be
> strictly ordered. The client state machine can certainly handle running
> these in parallel.
I'm not worried about the rpc calls themselves, I'm worried about the
other stuff in nfsd4_run_cb_work(), especially
nfsd4_process_cb_update().
It's been a while since I thought about it and maybe it'd be OK with a
little bit of extra locking.
--b.
> > Yes, there can be multiple cb_work's.
> >
>
> Yes, but each is effectively a separate work unit. I see no reason why
> we'd need to order them at all.
>
> --
> Jeff Layton <[email protected]>
On Wed, 2016-11-09 at 11:27 -0500, J. Bruce Fields wrote:
> On Wed, Nov 09, 2016 at 08:18:08AM -0500, Jeff Layton wrote:
> >
> > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote:
> > >
> > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> > > >
> > > >
> > > > Hello, Bruce.
> > > >
> > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > > > >
> > > > >
> > > > > Apologies, just cleaning out old mail and finding some I should have
> > > > > responded to long ago:
> > > > >
> > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> > > > > >
> > > > > >
> > > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per
> > > > > > nfsd4_callback instance and thus, it doesn't require execution ordering.
> > > > >
> > > > > What's "execution ordering"?
> > > > >
> >
> > AIUI, it means that jobs are always run in the order queued and are
> > serialized.
> >
> > >
> > > >
> > > > >
> > > > > We definitely do depend on the fact that at most one of these is running
> > > > > at a time.
> > > >
> >
> > We do?
> >
> > >
> > > >
> > > > If there can be multiple cb's and thus cb->cb_work's per callback_wq,
> > > > it'd need explicit ordering. Is that the case?
> > >
> >
> > These are basically client RPC tasks, and the cb_work just handles the
> > submission into the client RPC state machine. Just because we're running
> > several callbacks at the same time doesn't mean that they need to be
> > strictly ordered. The client state machine can certainly handle running
> > these in parallel.
>
> I'm not worried about the rpc calls themselves, I'm worried about the
> other stuff in nfsd4_run_cb_work(), especially
> nfsd4_process_cb_update().
>
> It's been a while since I thought about it and maybe it'd be OK with a
> little bit of extra locking.
>
> --b.
>
Ahh good point there. nfsd4_process_cb_update is a bit of a special
case, and I hadn't considered that.
I think we could use the cl_lock to protect most of the fields that are
affected there.
I'm not sure how to handle setup_callback_client though. Should we
serialize those calls so that we're only constructing one at a time and
have other threads wait on it? We could use a cl_flags bit for a
NFSD4_CLIENT_CB_CONSTRUCTING flag and serialize on those?
--
Jeff Layton <[email protected]>
On Wed, Nov 09, 2016 at 12:33:35PM -0500, Jeff Layton wrote:
> On Wed, 2016-11-09 at 11:27 -0500, J. Bruce Fields wrote:
> > On Wed, Nov 09, 2016 at 08:18:08AM -0500, Jeff Layton wrote:
> > >
> > > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote:
> > > >
> > > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote:
> > > > >
> > > > >
> > > > > Hello, Bruce.
> > > > >
> > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote:
> > > > > >
> > > > > >
> > > > > > Apologies, just cleaning out old mail and finding some I should have
> > > > > > responded to long ago:
> > > > > >
> > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote:
> > > > > > >
> > > > > > >
> > > > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per
> > > > > > > nfsd4_callback instance and thus, it doesn't require execution ordering.
> > > > > >
> > > > > > What's "execution ordering"?
> > > > > >
> > >
> > > AIUI, it means that jobs are always run in the order queued and are
> > > serialized.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > We definitely do depend on the fact that at most one of these is running
> > > > > > at a time.
> > > > >
> > >
> > > We do?
> > >
> > > >
> > > > >
> > > > > If there can be multiple cb's and thus cb->cb_work's per callback_wq,
> > > > > it'd need explicit ordering. Is that the case?
> > > >
> > >
> > > These are basically client RPC tasks, and the cb_work just handles the
> > > submission into the client RPC state machine. Just because we're running
> > > several callbacks at the same time doesn't mean that they need to be
> > > strictly ordered. The client state machine can certainly handle running
> > > these in parallel.
> >
> > I'm not worried about the rpc calls themselves, I'm worried about the
> > other stuff in nfsd4_run_cb_work(), especially
> > nfsd4_process_cb_update().
> >
> > It's been a while since I thought about it and maybe it'd be OK with a
> > little bit of extra locking.
> >
> > --b.
> >
>
> Ahh good point there. nfsd4_process_cb_update is a bit of a special
> case, and I hadn't considered that.
>
> I think we could use the cl_lock to protect most of the fields that are
> affected there.
>
> I'm not sure how to handle setup_callback_client though. Should we
> serialize those calls so that we're only constructing one at a time and
> have other threads wait on it? We could use a cl_flags bit for a
> NFSD4_CLIENT_CB_CONSTRUCTING flag and serialize on those?
For now I wish we could just like to continue assuming the workqueue
processes only one item at a time. Do we have that now, or do we need
to switch to (looking at workqueue.h...) alloc_ordered workqueue()?
--b.
On Wed, Nov 09, 2016 at 02:47:24PM -0500, J. Bruce Fields wrote:
> For now I wish we could just like to continue assuming the workqueue
> processes only one item at a time. Do we have that now, or do we need
> to switch to (looking at workqueue.h...) alloc_ordered workqueue()?
Oh, wait, I missed the _ordered_ in:
#define create_singlethread_workqueue(name) \
alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
So our existing create_singlethread_workqueue is fine--but we probably
don't need those flags:
--b.
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 211dc2aed8e1..eb78109d666c 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1061,7 +1061,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {
int nfsd4_create_callback_queue(void)
{
- callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
+ callback_wq = alloc_ordered_workqueue("nfsd4_callbacks", 0);
if (!callback_wq)
return -ENOMEM;
return 0;
On Wed, 2016-11-09 at 15:23 -0500, J. Bruce Fields wrote:
On Wed, Nov 09, 2016 at 02:47:24PM -0500, J. Bruce Fields wrote:
For now I wish we could just like to continue assuming the workqueue
processes only one item at a time. Do we have that now, or do we need
to switch to (looking at workqueue.h...) alloc_ordered workqueue()?
Oh, wait, I missed the _ordered_ in:
#define create_singlethread_workqueue(name) \
alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
So our existing create_singlethread_workqueue is fine--but we probably
don't need those flags:
--b.
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 211dc2aed8e1..eb78109d666c 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1061,7 +1061,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {
int nfsd4_create_callback_queue(void)
{
- callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
+ callback_wq = alloc_ordered_workqueue("nfsd4_callbacks", 0);
if (!callback_wq)
return -ENOMEM;
return 0;
That looks good to me. Eventually we could better serialize the handling
of the callback code and move to an unordered workqueue, but for now I
think Bruce is right that we have to keep it.
Reviewed-by: Jeff Layton <[email protected]>