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
