Re: [Cluster-devel] RFC: hold i_rwsem until aio completes
On Tue, Jan 14, 2020 at 05:12:13PM +0100, Christoph Hellwig wrote: > Hi all, > > Asynchronous read/write operations currently use a rather magic locking > scheme, were access to file data is normally protected using a rw_semaphore, > but if we are doing aio where the syscall returns to userspace before the > I/O has completed we also use an atomic_t to track the outstanding aio > ops. This scheme has lead to lots of subtle bugs in file systems where > didn't wait to the count to reach zero, and due to its adhoc nature also > means we have to serialize direct I/O writes that are smaller than the > file system block size. I've seen similar locking patterns quite a lot, enough I've thought about having a dedicated locking primitive to do it. It really wants to be a rwsem, but as here the rwsem rules don't allow it. The common pattern I'm looking at looks something like this: 'try begin read'() // aka down_read_trylock() /* The lockdep release hackery you describe, the rwsem remains read locked */ 'exit reader'() .. delegate unlock to work queue, timer, irq, etc .. in the new context: 're_enter reader'() // Get our lockdep tracking back 'end reader'() // aka up_read() vs a typical write side: 'begin write'() // aka down_write() /* There is no reason to unlock it before kfree of the rwsem memory. Somehow the user prevents any new down_read_trylock()'s */ 'abandon writer'() // The object will be kfree'd with a locked writer kfree() The typical goal is to provide an object destruction path that can serialize and fence all readers wherever they may be before proceeding to some synchronous destruction. Usually this gets open coded with some atomic/kref/refcount and a completion or wait queue. Often implemented wrongly, lacking the write favoring bias in the rwsem, and lacking any lockdep tracking on the naked completion. Not to discourage your patch, but to ask if we can make the solution more broadly applicable? Thanks, Jason
Re: [Cluster-devel] RFC: hold i_rwsem until aio completes
On Wed, Jan 15, 2020 at 03:33:47PM +0100, Peter Zijlstra wrote: > On Wed, Jan 15, 2020 at 09:24:28AM -0400, Jason Gunthorpe wrote: > > > I was interested because you are talking about allowing the read/write side > > of a rw sem to be held across a return to user space/etc, which is the > > same basic problem. > > No it is not; allowing the lock to be held across userspace doesn't > change the owner. This is a crucial difference, PI depends on there > being a distinct owner. That said, allowing the lock to be held across > userspace still breaks PI in that it completely wrecks the ability to > analyze the critical section. I'm not sure what you are contrasting? I was remarking that I see many places open code a rwsem using an atomic and a completion specifically because they need to do the things Christoph identified: > (1) no unlocking by another process than the one that acquired it > (2) no return to userspace with locks held As an example flow: obtain the read side lock, schedual a work queue, return to user space, and unlock the read side from the work queue. If we can make some general primative that addresses this then maybe those open coded places can convert as well? Regards, Jason
Re: [Cluster-devel] RFC: hold i_rwsem until aio completes
On Wed, Jan 15, 2020 at 07:56:14AM +0100, Christoph Hellwig wrote: > On Tue, Jan 14, 2020 at 03:27:00PM -0400, Jason Gunthorpe wrote: > > I've seen similar locking patterns quite a lot, enough I've thought > > about having a dedicated locking primitive to do it. It really wants > > to be a rwsem, but as here the rwsem rules don't allow it. > > > > The common pattern I'm looking at looks something like this: > > > > 'try begin read'() // aka down_read_trylock() > > > > /* The lockdep release hackery you describe, > > the rwsem remains read locked */ > > 'exit reader'() > > > > .. delegate unlock to work queue, timer, irq, etc .. > > > > in the new context: > > > > 're_enter reader'() // Get our lockdep tracking back > > > > 'end reader'() // aka up_read() > > > > vs a typical write side: > > > > 'begin write'() // aka down_write() > > > > /* There is no reason to unlock it before kfree of the rwsem memory. > > Somehow the user prevents any new down_read_trylock()'s */ > > 'abandon writer'() // The object will be kfree'd with a locked writer > > kfree() > > > > The typical goal is to provide an object destruction path that can > > serialize and fence all readers wherever they may be before proceeding > > to some synchronous destruction. > > > > Usually this gets open coded with some atomic/kref/refcount and a > > completion or wait queue. Often implemented wrongly, lacking the write > > favoring bias in the rwsem, and lacking any lockdep tracking on the > > naked completion. > > > > Not to discourage your patch, but to ask if we can make the solution > > more broadly applicable? > > Your requirement seems a little different, and in fact in many ways > similar to the percpu_ref primitive. I was interested because you are talking about allowing the read/write side of a rw sem to be held across a return to user space/etc, which is the same basic problem. precpu refcount looks more like a typical refcount with a release that is called by whatever context does the final put. The point above is to basically move the release of a refcount into a synchrnous path by introducing some barrier to wait for the refcount to go to zero. In the above the barrier is the down_write() as it is really closer to a rwsem than a refcount. Thanks, Jason
Re: [Cluster-devel] RFC: hold i_rwsem until aio completes
On Wed, Jan 15, 2020 at 04:36:14PM +0100, Christoph Hellwig wrote: > synchronous and currently hack that up, so a version of the percpu_ref > that actually waits for the other references to away like we hacked > up various places seems to exactly suit your requirements. Ah, yes, sounds like a similar goal, many cases I'm thinking about also hack up a kref to trigger a completion to make it synchronous. Jason
Re: [Cluster-devel] [PATCH 11/33] tcp: tcp_sock_set_nodelay
On Wed, May 13, 2020 at 08:26:26AM +0200, Christoph Hellwig wrote: > Add a helper to directly set the TCP_NODELAY sockopt from kernel space > without going through a fake uaccess. Cleanup the callers to avoid > pointless wrappers now that this is a simple function call. > > Signed-off-by: Christoph Hellwig > --- > drivers/block/drbd/drbd_int.h | 7 > drivers/block/drbd/drbd_main.c| 2 +- > drivers/block/drbd/drbd_receiver.c| 4 +-- > drivers/infiniband/sw/siw/siw_cm.c| 24 +++--- > drivers/nvme/host/tcp.c | 9 +- > drivers/nvme/target/tcp.c | 12 ++- > drivers/target/iscsi/iscsi_target_login.c | 15 ++--- > fs/cifs/connect.c | 10 ++ > fs/dlm/lowcomms.c | 8 ++--- > fs/ocfs2/cluster/tcp.c| 20 ++-- > include/linux/tcp.h | 1 + > net/ceph/messenger.c | 11 ++- > net/ipv4/tcp.c| 39 +++ > net/rds/tcp.c | 11 +-- > net/rds/tcp.h | 1 - > net/rds/tcp_listen.c | 2 +- > 16 files changed, 49 insertions(+), 127 deletions(-) No problem with the siw change Acked-by: Jason Gunthorpe Jason
Re: [Cluster-devel] [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote: > IB/hfi1: Fix fall-through warnings for Clang > IB/mlx4: Fix fall-through warnings for Clang > IB/qedr: Fix fall-through warnings for Clang > RDMA/mlx5: Fix fall-through warnings for Clang I picked these four to the rdma tree, thanks Jason
