lkml.org 
[lkml]   [2018]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 13/17] staging: lustre: remove 'ptlrpc_thread usage' for sai_agl_thread
Date
On Mar 1, 2018, at 16:31, NeilBrown <neilb@suse.com> wrote:
>
> Lustre has a 'struct ptlrpc_thread' which provides
> control functionality wrapped around kthreads.
> None of the functionality used in statahead.c requires
> ptlrcp_thread - it can all be done directly with kthreads.
>
> So discard the ptlrpc_thread and just use a task_struct directly.
>
> One particular change worth noting is that in the current
> code, the thread performs some start-up actions and then
> signals that it is ready to go. In the new code, the thread
> is first created, then the startup actions are perform, then
> the thread is woken up. This means there is no need to wait
> any more than kthread_create() already waits.
>
> Signed-off-by: NeilBrown <neilb@suse.com>

Looks reasonable, but one minor comment inline below. Not enough to
make me think the patch is bad, just a minor inefficiency...

Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>

I've also CC'd Fan Yong, who is the author of this code in case
he has any comments.

> diff --git a/drivers/staging/lustre/lustre/llite/statahead.c b/drivers/staging/lustre/lustre/llite/statahead.c
> index ba00881a5745..39241b952bf4 100644
> --- a/drivers/staging/lustre/lustre/llite/statahead.c
> +++ b/drivers/staging/lustre/lustre/llite/statahead.c
> @@ -861,35 +860,13 @@ static int ll_agl_thread(void *arg)
> struct inode *dir = d_inode(parent);
> struct ll_inode_info *plli = ll_i2info(dir);
> struct ll_inode_info *clli;
> - struct ll_sb_info *sbi = ll_i2sbi(dir);
> - struct ll_statahead_info *sai;
> - struct ptlrpc_thread *thread;
> + /* We already own this reference, so it is safe to take it without a lock. */
> + struct ll_statahead_info *sai = plli->lli_sai;
>
> - sai = ll_sai_get(dir);

Here we used to grab a reference to "sai" from the directory, but you
get it in the calling thread now...

> @@ -937,16 +917,22 @@ static void ll_start_agl(struct dentry *parent, struct ll_statahead_info *sai)
> sai, parent);
>
> plli = ll_i2info(d_inode(parent));
> + task = kthread_create(ll_agl_thread, parent, "ll_agl_%u",
> + plli->lli_opendir_pid);
> if (IS_ERR(task)) {
> CERROR("can't start ll_agl thread, rc: %ld\n", PTR_ERR(task));
> return;
> }
>
> + sai->sai_agl_task = task;
> + atomic_inc(&ll_i2sbi(d_inode(parent))->ll_agl_total);
> + spin_lock(&plli->lli_agl_lock);
> + sai->sai_agl_valid = 1;
> + spin_unlock(&plli->lli_agl_lock);
> + /* Get an extra reference that the thread holds */
> + ll_sai_get(d_inode(parent));

Here you get the extra reference, but we already have the pointer to
"sai", do going through "parent->d_inode->lli->lli_sai" to get "sai"
again seems convoluted. One option is atomic_inc(&sai->sai_refcount),
but given that this is done only once per "ls" call I don't think it
is a huge deal, and not more work than was done before.

Cheers, Andreas

> +
> + wake_up_process(task);
> }
>
> /* statahead thread main function */
> @@ -958,7 +944,6 @@ static int ll_statahead_thread(void *arg)
> struct ll_sb_info *sbi = ll_i2sbi(dir);
> struct ll_statahead_info *sai;
> struct ptlrpc_thread *sa_thread;
> - struct ptlrpc_thread *agl_thread;
> struct page *page = NULL;
> __u64 pos = 0;
> int first = 0;
> @@ -967,7 +952,6 @@ static int ll_statahead_thread(void *arg)
>
> sai = ll_sai_get(dir);
> sa_thread = &sai->sai_thread;
> - agl_thread = &sai->sai_agl_thread;
> sa_thread->t_pid = current_pid();
> CDEBUG(D_READA, "statahead thread starting: sai %p, parent %pd\n",
> sai, parent);
> @@ -1129,21 +1113,13 @@ static int ll_statahead_thread(void *arg)
> sa_handle_callback(sai);
> }
> out:
> - if (sai->sai_agl_valid) {
> - spin_lock(&lli->lli_agl_lock);
> - thread_set_flags(agl_thread, SVC_STOPPING);
> - spin_unlock(&lli->lli_agl_lock);
> - wake_up(&agl_thread->t_ctl_waitq);
> + if (sai->sai_agl_task) {
> + kthread_stop(sai->sai_agl_task);
>
> CDEBUG(D_READA, "stop agl thread: sai %p pid %u\n",
> - sai, (unsigned int)agl_thread->t_pid);
> - wait_event_idle(agl_thread->t_ctl_waitq,
> - thread_is_stopped(agl_thread));
> - } else {
> - /* Set agl_thread flags anyway. */
> - thread_set_flags(agl_thread, SVC_STOPPED);
> + sai, (unsigned int)sai->sai_agl_task->pid);
> + sai->sai_agl_task = NULL;
> }
> -
> /*
> * wait for inflight statahead RPCs to finish, and then we can free sai
> * safely because statahead RPC will access sai data
>
>

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation







\
 
 \ /
  Last update: 2018-03-09 01:12    [W:0.159 / U:0.660 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site