Re: [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Oct 06, 2020 at 09:04:18AM -0500, Eric Sandeen wrote:
> 
> 
> On 10/6/20 5:50 AM, Pavel Reichl wrote:
> >> Also, we're not really releasing the lock itself here, right?  We're
> >> merely updating lockdep's bookkeepping so that the worker can make
> >> itself look like the lock owner (to lockdep, anyway).
> > Hmm...I'm afraid I don't follow - yes we are doing this to satisfy lockdep's bookkeeping,
> > however we actually do this by releasing the lock in one kernel thread and acquiring it in another.
> 
> it's the difference between actually releasing the lock itself, and
> telling lockdep that we're releasing the "ownership" of the lock for tracking
> purposes; I agree that "rwsem_release" is a bit confusingly named.

Yes.

> > 
> >> Does this exist as a helper anywhere in the kernel?  I don't really like
> >> XFS poking into the rw_semaphore innards, though I concede that this
> >> lock transferring dance is probably pretty rare.
> > I'll try to look for it.
> > 
> 
> Other code I see just calls rwsem_release directly - ocfs2, jbd2, kernfs etc.
> 
> I think a clearer comment might suffice, not sure what Darrick thinks, maybe something
> like this:
> 
> +	/*
> +	 * Let lockdep know that we won't own i_lock when we hand off
> +	 * to the worker thread
> +	 */

	/*
	 * Update lockdep's ownership information to reflect that we
	 * will be transferring the ilock from this thread to the
	 * worker.
	 */

> +	rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
>  	queue_work(xfs_alloc_wq, &args.work);
> +
>  	wait_for_completion(&done);
> +	/* We own the i_lock again */

	/*
	 * Update lockdep's lock ownership information to point to
	 * this thread as the lock owner now that the worker item is
	 * done.
	 */

Perhaps?

--D

> +	rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
> 
> and similar comments in the worker:
> 
> +	/* Let lockdep know that we own the i_lock for now */
> +	rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
> ...
> 
> etc
> 
> -Eric