On Tue, Nov 8, 2022 at 10:16 PM Tom Talpey <tom@xxxxxxxxxx> wrote: > > On 11/8/2022 11:33 AM, Shyam Prasad N wrote: > > Came across a couple of race conditions between cifsd and i/o threads. > > Fixed them here. Please review the changes. > > > > This change should also ensure that binding session requests do not > > race with the primary session request. > > > > https://github.com/sprasad-microsoft/smb3-kernel-client/commit/c4ed4d985488640469acfc621bed5c3a55d67ac6.patch > > > > Copy/pasting from that page: > > @@ -4111,6 +4111,16 @@ cifs_setup_session(const unsigned int xid, struct > cifs_ses *ses, > else > scnprintf(ses->ip_addr, sizeof(ses->ip_addr), "%pI4", &addr->sin_addr); > > + /* > + * if primary channel is still being setup, > + * we cannot bind to it. try again after sometime > + */ > + if (ses->ses_status == SES_IN_SETUP) { > + spin_unlock(&ses->ses_lock); > + msleep(10); > + return -EAGAIN; > + } > + > if (ses->ses_status != SES_GOOD && > ses->ses_status != SES_NEW && > ses->ses_status != SES_NEED_RECON) { > > Sleeping for 10ms is really an ugly approach. Apart from making a > completely arbitrary timing choice, blocking the caller uninterruptibly > is pretty rude. And if the existing setup fails, this will blindly > retry it, immediately. > > Can't this take a more formal queued approach, with a proper wakeup on > success, or bail out on interrupt, timeout or fail? You make a good point. I thought this was not very clean too. But took this approach to keep it simple. Let me explore the waiting approach. Thanks for the review. :) > > Tom. -- Regards, Shyam
