| Message ID | 1533313003-30186-1-git-send-email-sreekanth.reddy@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] mpt3sas: correct reset of smid while clearing scsi tracker | expand |
On Fri, 2018-08-03 at 12:16 -0400, Sreekanth Reddy wrote: > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..2c5a5b4 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -1702,6 +1702,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > return NULL; > > chain_req = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; > + > + /* > + * Added memory barrier to make sure that correct chain tracker > + * is retrieved before incrementing the smid pool's chain_offset > + * value in chain lookup table. > + */ > + smp_mb(); > atomic_inc(&ioc->chain_lookup[smid - 1].chain_offset); > return chain_req; > } > @@ -3283,8 +3290,15 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); > + > + /* > + * Added memory barrier to make sure that smid is set to zero > + * only after resetting corresponding smid pool's chain_offset to zero > + * in chain lookup table. > + */ > + smp_mb(); > + st->smid = 0; > } Thanks for having addressed previous review comments. Hence: Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> However, I'm not yet convinced that this patch is sufficient to fix the race between _base_get_chain_buffer_tracker() and mpt3sas_base_clear_st(). Can e.g. the atomic_set() that resets chain_offset occur after it has been read by _base_get_chain_buffer_tracker() and before that function increments the chain_offset member variable? Thanks, Bart.
On Fri, Aug 3, 2018 at 10:02 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Fri, 2018-08-03 at 12:16 -0400, Sreekanth Reddy wrote: >> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c >> index 902610d..2c5a5b4 100644 >> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >> @@ -1702,6 +1702,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) >> return NULL; >> >> chain_req = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; >> + >> + /* >> + * Added memory barrier to make sure that correct chain tracker >> + * is retrieved before incrementing the smid pool's chain_offset >> + * value in chain lookup table. >> + */ >> + smp_mb(); >> atomic_inc(&ioc->chain_lookup[smid - 1].chain_offset); >> return chain_req; >> } >> @@ -3283,8 +3290,15 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, >> return; >> st->cb_idx = 0xFF; >> st->direct_io = 0; >> - st->smid = 0; >> atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); >> + >> + /* >> + * Added memory barrier to make sure that smid is set to zero >> + * only after resetting corresponding smid pool's chain_offset to zero >> + * in chain lookup table. >> + */ >> + smp_mb(); >> + st->smid = 0; >> } > > Thanks for having addressed previous review comments. Hence: > > Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> > > However, I'm not yet convinced that this patch is sufficient to fix the race > between _base_get_chain_buffer_tracker() and mpt3sas_base_clear_st(). Can e.g. > the atomic_set() that resets chain_offset occur after it has been read by > _base_get_chain_buffer_tracker() and before that function increments the > chain_offset member variable? > No Bart, their is no race condition here. Since chain lookup table entry is uniquely accessed using smid value. And this smid (which is scmd->request->tag +1) is unique for each IO request. And _base_get_chain_buffer_tracker() function is called only during IO submission time (i.e. before submitting the IO to the IOC firmware) and mpt3sas_base_clear_st() function is called in the ISR (i.e during IO completion) path. So for a particular IO these two functions called at two different instances of time. Thanks, Sreekanth > Thanks, > > Bart. >
On Sat, 2018-08-04 at 18:56 +0530, Sreekanth Reddy wrote: > No Bart, their is no race condition here. Since chain lookup table > entry is uniquely accessed using smid value. And this smid (which is > scmd->request->tag +1) is unique for each IO request. And > _base_get_chain_buffer_tracker() function is called only during IO > submission time (i.e. before submitting the IO to the IOC firmware) > and mpt3sas_base_clear_st() function is called in the ISR (i.e during > IO completion) path. So for a particular IO these two functions called > at two different instances of time. Hello Sreekanth, In mpt3sas_base_get_smid_scsiio() I found the following code: struct scsiio_tracker *request = scsi_cmd_priv(scmd); u16 smid = scmd->request->tag + 1; request->smid = smid; That confirms what you wrote about smid being unique for each I/O request. However, this also raises new questions. As far as I can see all code in the I/O path that accesses the chain_lookup[] array uses index smid - 1. The patch at the start of this e-mail thread modifies two functions, namely mpt3sas_remove_dead_ioc_func() and mpt3sas_base_clear_st(). Since the chain_lookup[] array is indexed with the smid and hence contains request- private data, how is it even possible that these functions race against each other? Is there perhaps code that accesses the chain_lookup[] array and that is called from another context than the I/O completion path? Is the patch at the start of this e-mail thread the result of a theoretical concern or of a real failure? In the latter case, which sequence of actions triggered the failure? The answer to this question should already have been mentioned in the description of v1 of this patch. Thanks, Bart.
On Mon, Aug 6, 2018 at 11:41 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Sat, 2018-08-04 at 18:56 +0530, Sreekanth Reddy wrote: >> No Bart, their is no race condition here. Since chain lookup table >> entry is uniquely accessed using smid value. And this smid (which is >> scmd->request->tag +1) is unique for each IO request. And >> _base_get_chain_buffer_tracker() function is called only during IO >> submission time (i.e. before submitting the IO to the IOC firmware) >> and mpt3sas_base_clear_st() function is called in the ISR (i.e during >> IO completion) path. So for a particular IO these two functions called >> at two different instances of time. > > Hello Sreekanth, > > In mpt3sas_base_get_smid_scsiio() I found the following code: > > struct scsiio_tracker *request = scsi_cmd_priv(scmd); > u16 smid = scmd->request->tag + 1; > request->smid = smid; > > That confirms what you wrote about smid being unique for each I/O request. > However, this also raises new questions. As far as I can see all code in > the I/O path that accesses the chain_lookup[] array uses index smid - 1. Our IOC firmware always requires smid to be >=1, zero is illegal smid for the IOC firmware. Hence while framing MPT SCSI IO request driver always make sure that smid to be >=1, so it uses smid as tag + 1. where as chain_lookup[] starts with index zero, So driver is accessing this chain_lookup[] with smid-1 index. > The patch at the start of this e-mail thread modifies two functions, namely > mpt3sas_remove_dead_ioc_func() and mpt3sas_base_clear_st(). This patch modifies in two functions namely '_base_get_chain_buffer_tracker()' & 'mpt3sas_base_clear_st()' not in the mpt3sas_remove_dead_ioc_func(), not sure why 'git format-patch' is showing 'mpt3sas_remove_dead_ioc_func' function name in the patch. This function _base_get_chain_buffer_tracker() is called only during the IO submission time and mpt3sas_base_clear_st() is called during IO completion time and hence I don't see any race condition here. Currently this patch is very much needed, so please consider this patch. May be we can start new email thread to discuss on the possible race conditions you are observing and I can clear your quires or I can fix them as on we can find the real race conditions. Thanks, Sreekanth Since the > chain_lookup[] array is indexed with the smid and hence contains request- > private data, how is it even possible that these functions race against > each other? Is there perhaps code that accesses the chain_lookup[] array > and that is called from another context than the I/O completion path? > > Is the patch at the start of this e-mail thread the result of a theoretical > concern or of a real failure? In the latter case, which sequence of actions > triggered the failure? The answer to this question should already have been > mentioned in the description of v1 of this patch. > > Thanks, > > Bart.
On Tue, 2018-08-07 at 12:10 +0530, Sreekanth Reddy wrote: > This function _base_get_chain_buffer_tracker() is called only during > the IO submission time and mpt3sas_base_clear_st() is called during IO > completion time and hence I don't see any race condition here. > > Currently this patch is very much needed, so please consider this > patch. May be we can start new email thread to discuss on the possible > race conditions you are observing and I can clear your quires or I can > fix them as on we can find the real race conditions. Hello Sreekanth, How can this patch be necessary if there are no races between I/O submission and I/O completion? Changing the order of two memory writes only makes sense if there is a race condition involved. Since the block layer allocates a request tag before scsih_qcmd() is called and since mpt3sas_base_free_smid() is called before scmd->scsi_done(scmd) in _scsih_io_done(), the request tag is freed after the smid has been freed when I/O completes in a normal way. So there shouldn't be a race condition in that scenario. By the way, you have not answered my question about why development of this patch started. Does this patch address a theoretical concern or a real bug? In the latter case, which scenario triggers the addressed bug? If you want this patch to go upstream you should be able to explain why you think it is useful, and that description should be included in the patch description. Thanks, Bart.
On Tue, Aug 7, 2018 at 7:29 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Tue, 2018-08-07 at 12:10 +0530, Sreekanth Reddy wrote: >> This function _base_get_chain_buffer_tracker() is called only during >> the IO submission time and mpt3sas_base_clear_st() is called during IO >> completion time and hence I don't see any race condition here. >> >> Currently this patch is very much needed, so please consider this >> patch. May be we can start new email thread to discuss on the possible >> race conditions you are observing and I can clear your quires or I can >> fix them as on we can find the real race conditions. > > Hello Sreekanth, > > How can this patch be necessary if there are no races between I/O submission > and I/O completion? Changing the order of two memory writes only makes sense > if there is a race condition involved. Since the block layer allocates a > request tag before scsih_qcmd() is called and since mpt3sas_base_free_smid() > is called before scmd->scsi_done(scmd) in _scsih_io_done(), the request tag > is freed after the smid has been freed when I/O completes in a normal way. > So there shouldn't be a race condition in that scenario. > > By the way, you have not answered my question about why development of this > patch started. Does this patch address a theoretical concern or a real bug? > In the latter case, which scenario triggers the addressed bug? If you want > this patch to go upstream you should be able to explain why you think it is > useful, and that description should be included in the patch description. Bart, The main intention of this patch to reset the smid to zero after resetting the corresponding smid entry's chain_offset to zero. While posting "mpt3sas: Fix calltrace observed while running IO & reset" patch I have done manual mistake that I reset the smid to zero before resetting the chain_offset which is wrong. Due to which driver always's reset the chain_offset of zero th entry of chain_lookup[] table which is wrong and hence I am correcting it with this patch. After that you asked me to add memory barriers and hence I have added memory barriers in subsequent versions of the patch. Thanks, Sreekanth > > Thanks, > > Bart. >
On Tue, 2018-08-07 at 20:03 +0530, Sreekanth Reddy wrote: > The main intention of this patch to reset the smid to zero after > resetting the corresponding smid entry's chain_offset to zero. While > posting "mpt3sas: Fix calltrace observed while running IO & reset" > patch I have done manual mistake that I reset the smid to zero before > resetting the chain_offset which is wrong. Due to which driver > always's reset the chain_offset of zero th entry of chain_lookup[] > table which is wrong and hence I am correcting it with this patch. > > After that you asked me to add memory barriers and hence I have added > memory barriers in subsequent versions of the patch. Hello Sreekanth, Please answer the questions I already asked you twice instead of sidestepping these. Thanks, Bart.
On Tue, Aug 7, 2018 at 8:27 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Tue, 2018-08-07 at 20:03 +0530, Sreekanth Reddy wrote: >> The main intention of this patch to reset the smid to zero after >> resetting the corresponding smid entry's chain_offset to zero. While >> posting "mpt3sas: Fix calltrace observed while running IO & reset" >> patch I have done manual mistake that I reset the smid to zero before >> resetting the chain_offset which is wrong. Due to which driver >> always's reset the chain_offset of zero th entry of chain_lookup[] >> table which is wrong and hence I am correcting it with this patch. >> >> After that you asked me to add memory barriers and hence I have added >> memory barriers in subsequent versions of the patch. > > Hello Sreekanth, > > Please answer the questions I already asked you twice instead of sidestepping > these. Here I am copying your quires from your previous mails and I am replaying inline.. In mpt3sas_base_get_smid_scsiio() I found the following code: struct scsiio_tracker *request = scsi_cmd_priv(scmd); u16 smid = scmd->request->tag + 1; request->smid = smid; That confirms what you wrote about smid being unique for each I/O request. However, this also raises new questions. As far as I can see all code in the I/O path that accesses the chain_lookup[] array uses index smid - 1. [Sreekanth] Our IOC firmware always requires smid to be >=1, zero is illegal smid for the IOC firmware. Hence while framing MPT SCSI IO request driver always make sure that smid to be >=1, so it uses smid as tag + 1. where as chain_lookup[] starts with index zero, So driver is accessing this chain_lookup[] with smid-1 index. The patch at the start of this e-mail thread modifies two functions, namely mpt3sas_remove_dead_ioc_func() and mpt3sas_base_clear_st(). Since the chain_lookup[] array is indexed with the smid and hence contains request- private data, how is it even possible that these functions race against each other? [Sreekanth] This patch modifies in two functions namely '_base_get_chain_buffer_tracker()' & 'mpt3sas_base_clear_st()' not in the mpt3sas_remove_dead_ioc_func(). Their is no race between mpt3sas_remove_dead_ioc_func() & mpt3sas_base_clear_st(). Also their is no race between _base_get_chain_buffer_tracker() & mpt3sas_base_clear_st(), since for a particular IO _base_get_chain_buffer_tracker() is called during IO submission time for getting required chain buffer and mpt3sas_base_clear_st() is called to free the used chain buffers in the IO completion path. Also we have pre-allocated required chain buffers for each IO request and hence no need to main any list and their is no race between these two functions. Is there perhaps code that accesses the chain_lookup[] array and that is called from another context than the I/O completion path? [Sreekanth] No, it is called only from IO submission time. Is the patch at the start of this e-mail thread the result of a theoretical concern or of a real failure? In the latter case, which sequence of actions triggered the failure? The answer to this question should already have been mentioned in the description of v1 of this patch [Sreekanth] The main intention of this patch to reset the smid to zero after resetting the corresponding smid entry's chain_offset to zero in chain_lookup[]. While posting "mpt3sas: Fix calltrace observed while running IO & reset" patch I have done manual mistake that I reset the smid to zero before resetting the chain_offset which is wrong. Due to which driver always's reset the chain_offset of zero th entry of chain_lookup[] table which is wrong and we may see that scmd's will be returned from scsih_qcmd() with "host busy" status due to unavailability of chain buffers (as driver is resetting the wrong smid's i.e. zero th smid entry's chain_offset instead of correct smid entry's chain_offset) and IO's will be in hung state . So, I am correcting it with this patch. After that you asked me to add memory barriers and hence I have added memory barriers in subsequent versions of the patch. How can this patch be necessary if there are no races between I/O submission and I/O completion? [Sreekanth] As I explained above in this patch I am resetting the smid variable to zero only after resetting the smid's chain_offset in chain_lookup[] table. Due to manual mistake earlier I am resetting the smid variable to zero before resetting the correct smid entry's chain_offset in the chain_lookup[] table, which is wrong and I am correcting it with this patch. Changing the order of two memory writes only makes sense if there is a race condition involved. Since the block layer allocates a request tag before scsih_qcmd() is called and since mpt3sas_base_free_smid() is called before scmd->scsi_done(scmd) in _scsih_io_done(), the request tag is freed after the smid has been freed when I/O completes in a normal way. So there shouldn't be a race condition in that scenario. [Sreekanth] yes their is no race condition in this scenario. By the way, you have not answered my question about why development of this patch started. Does this patch address a theoretical concern or a real bug? [Sreekanth] As I explained above while submitting "mpt3sas: Fix calltrace observed while running IO & reset" patch I have made a manual mistake that I am resetting the smid variable in wrong line in mpt3sas_base_clear_st() function and I am correcting it with this patch. In the latter case, which scenario triggers the addressed bug? [Sreekanth] Due to this manual mistake IO will be in hung state as driver will return the scmd's with host busy status due to unavailability of chain buffers after running IO for some time. If you want this patch to go upstream you should be able to explain why you think it is useful, and that description should be included in the patch description. [Sreekanth] In the patch description I mentioned that I have done a manual mistake and I am correcting with this patch. Hope I have answered all of your quires. Thanks, Sreekanth > > Thanks, > > Bart. >
On Tue, 2018-08-07 at 21:46 +0530, Sreekanth Reddy wrote: > [Sreekanth] In the patch description I mentioned that I have done a > manual mistake and I am correcting with this patch. > > Hope I have answered all of your quires. Not yet unfortunately. Why do you consider the current implementation a wrong? Why is the patch at the start of this e-mail thread considered a fix? Which behavior changes due to this patch? If this patch is a bug fix, how can the bug be triggered and why is this patch considered the right way to fix that bug? Thanks, Bart.
On Wed, Aug 8, 2018 at 1:27 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Tue, 2018-08-07 at 21:46 +0530, Sreekanth Reddy wrote: >> [Sreekanth] In the patch description I mentioned that I have done a >> manual mistake and I am correcting with this patch. >> >> Hope I have answered all of your quires. > > Not yet unfortunately. Why do you consider the current implementation a > wrong? In function mpt3sas_base_clear_st(), st->cb_idx = 0xFF; st->direct_io = 0; st->smid = 0; atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); In the above code we can see that smid is reset to zero before resetting the chain_offset to zero in chain_lookup[] table for corresponding IO request entry (i.e. smid -1 th entry). So in the above code driver is logically resetting the chain_offset to zero for the -1th entry of the chain_lookup[] table for all the IO requests, which I am say it wrong. I am saying that above code should be like below, st->cb_idx = 0xFF; st->direct_io = 0; atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); st->smid = 0; I.e. reset the smid variable to zero only after resetting the chain_offset for the corresponding IO request (i.e. for smid -1 th entry) of the chain_lookup[] table. > Why is the patch at the start of this e-mail thread considered a > fix? Which behavior changes due to this patch? If this patch is a bug fix, > how can the bug be triggered and why is this patch considered the right way > to fix that bug? See while posting the patch "mpt3sas: Fix calltrace observed while running IO & reset" in hurry I made a manual mistake that I added "st->smid = 0" line before "atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0);" line in mpt3sas_base_clear_st() function instead I should have added "st->smid = 0" line after the atomic set line. Which I am correcting my mistake with this patch. Thanks, Sreekanth > > Thanks, > > Bart. >
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 902610d..2c5a5b4 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -1702,6 +1702,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) return NULL; chain_req = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; + + /* + * Added memory barrier to make sure that correct chain tracker + * is retrieved before incrementing the smid pool's chain_offset + * value in chain lookup table. + */ + smp_mb(); atomic_inc(&ioc->chain_lookup[smid - 1].chain_offset); return chain_req; } @@ -3283,8 +3290,15 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, return; st->cb_idx = 0xFF; st->direct_io = 0; - st->smid = 0; atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); + + /* + * Added memory barrier to make sure that smid is set to zero + * only after resetting corresponding smid pool's chain_offset to zero + * in chain lookup table. + */ + smp_mb(); + st->smid = 0; } /**
In mpt3sas_base_clear_st() function smid value is reset in wrong line, i.e. driver should reset smid value to zero after decrementing chain_offset counter in chain_lookup table but in current code, driver is resetting smid value before decrementing the chain_offset counter. Which we are correcting with this patch. v1 changelog: Added memory barriers before & after atomic_set() API. v2 changelog: Added proper comments to describe the need of using smp_mb__before_atomic() & smp_mb__after_atomic() APIs in the driver before calling these APIs. v3 changelog: Replaced smp_mb__before_atomic() & smp_mb__after_atomic() APIs with smp_mb() APIs. Used smp_mb() API in mpt3sas_base_clear_st() to make sure that smid is reset to zero only after corresponding smid pool's chain_offset in chain lookup table is reset to zero. Used smp_mb() API in _base_get_chain_buffer_tracker() to make sure that proper chain tracker is retrieved from the corresponding smid's pool from chain table before incrementing smid pool's chain offset value. Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)