2012/2/28 Jeff Layton <jlayton@xxxxxxxxxx>: > On Tue, 28 Feb 2012 13:38:46 +0400 > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > >> 2012/2/28 Steve French <smfrench@xxxxxxxxx>: >> > I got a chance to look through this patch set in more detail on the way >> > to the SMB2.2 test event. I liked many of the patches, but this one >> > seems to complicate, rather than simplify by adding a spinlock >> > to what used to be an atomic_inc. There is also a possibility that this >> > spinlock will become hot. I don't mind moving the atomic_inc inside the >> > new dec_in_flight macro though to make it easier in the future. >> > >> >> The idea was to simplify the usage of inFlight value. Now it is >> protected by both spin_lock and atomic mechanism in one case and with >> only atomic in another - I think it's rather complicated. The patch >> gets rid of one of the protection mechanism (atomic) and use only >> spin_lock - this spin_lock is used further for protecting oplock and >> echo specific variables as well. >> > > The problem here is that the locking around this variable is unclear: > > If you need a spinlock to access/change it, then why use an atomic? If > an atomic is sufficient then why use the spinlock at all? > > I don't much care what scheme you end up using for this, but if you > really need a belt-and-suspenders approach (atomic + spinlock), then > the reasoning for it needs to be clearly documented. We mere mortals > must understand how it's supposed to work so we don't break it later. I don't know how the existing approach comes the tree but I guess that it needs to be atomic to easily decrement and read this variable and needs to be surrounded by spinlocks in wait_for_free_request to make sure we are not incrementing the value that is already >= cifs_max_pending. The whole scheme is rather complicated of course - so, the goal of this patch is to make it more understandable and simple (only one protection mechanism - spin_lock). -- Best regards, Pavel Shilovsky. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
