On Sun, Jul 25, 2010 at 8:36 PM, David Dillow <[email protected]> wrote: > > On Sun, 2010-07-25 at 18:12 +0200, Bart Van Assche wrote: > > In the current implementation of ib_srp the req_lim field of > > struct srp_target_port can be manipulated in a non-atomic way by > > more than one CPU at a time: one CPU can be modifying req_lim in > > function srp_process_rsp() while another CPU can concurrently be > > decrementing req_lim in function __srp_get_tx_iu(). This is a > > race condition which can result in incorrect manipulation of the > > req_lim field. The patch below fixes this race condition by > > converting all manipulations of req_lim into atomic operations. > > This is not needed -- all modifications of target->req_lim are protected > by scsi_dev->host_lock. It is held implicitly in srp_queuecommand() by > the SCSI mid-layer, and we take that lock before modifying req_lim > elsewhere -- or we're initializing and there won't be concurrent access.
It would be helpful for anyone who is reviewing or modifying the SRP initiator source code if the locking policy was documented. As an example, in kernels 2.6.33 and before there was a single notification processing function srp_completion() that incremented target->tx_tail non-atomically and without protection of scsi_host->host_lock. It was not possible to tell from the source code comments whether or not this access pattern was intentional. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
