| Message ID | 20181107201055.25883-3-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: fix panics seen with error injection | expand |
On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > on the li, which clears the LI_FAILED flag, but also drops the ref on > the buf. Since it isn't on a IO list at this point this could very well > be the last ref on the buf, which wreaks havoc when we go to add the buf > to the delwrite list. Fix this by holding a ref on the buf before we > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > doesn't disappear before we're able to clear the error and add it to the > delwri list. This fixes the panics I was seeing with error injection. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/xfs/xfs_inode_item.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > index fa1c4fe2ffbf..df49adf1989c 100644 > --- a/fs/xfs/xfs_inode_item.c > +++ b/fs/xfs/xfs_inode_item.c > @@ -503,13 +503,16 @@ xfs_inode_item_push( > * previously. Resubmit the buffer for IO. > */ > if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { > - if (!xfs_buf_trylock(bp)) > + xfs_buf_hold(bp); > + if (!xfs_buf_trylock(bp)) { > + xfs_buf_rele(bp); > return XFS_ITEM_LOCKED; > + } > > if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) > rval = XFS_ITEM_FLUSHING; > > - xfs_buf_unlock(bp); > + xfs_buf_relse(bp); > return rval; > } This just doesn't smell right to me. /me rummages around in the code Ok, so I think the bug is in xfs_buf_resubmit_failed_buffers() in that it removes all the "failed" reference counts from the buffer before it adds the delwri reference count back to the buffer. Taking a high level reference count like above just papers over the transient reference counting error in xfs_buf_resubmit_failed_buffers(). It also fails to fix the other xfs_buf_resubmit_failed_buffers() caller, which has exactly the same problem (dquot writeback). Perhaps something like the patch below? Cheers, Dave.
On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > > on the li, which clears the LI_FAILED flag, but also drops the ref on > > the buf. Since it isn't on a IO list at this point this could very well > > be the last ref on the buf, which wreaks havoc when we go to add the buf > > to the delwrite list. Fix this by holding a ref on the buf before we > > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > > doesn't disappear before we're able to clear the error and add it to the > > delwri list. This fixes the panics I was seeing with error injection. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/xfs/xfs_inode_item.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > > index fa1c4fe2ffbf..df49adf1989c 100644 > > --- a/fs/xfs/xfs_inode_item.c > > +++ b/fs/xfs/xfs_inode_item.c > > @@ -503,13 +503,16 @@ xfs_inode_item_push( > > * previously. Resubmit the buffer for IO. > > */ > > if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { > > - if (!xfs_buf_trylock(bp)) > > + xfs_buf_hold(bp); > > + if (!xfs_buf_trylock(bp)) { > > + xfs_buf_rele(bp); > > return XFS_ITEM_LOCKED; > > + } > > > > if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) > > rval = XFS_ITEM_FLUSHING; > > > > - xfs_buf_unlock(bp); > > + xfs_buf_relse(bp); > > return rval; > > } > > This just doesn't smell right to me. > > /me rummages around in the code > > Ok, so I think the bug is in xfs_buf_resubmit_failed_buffers() in > that it removes all the "failed" reference counts from the buffer > before it adds the delwri reference count back to the buffer. Taking > a high level reference count like above just papers over the > transient reference counting error in > xfs_buf_resubmit_failed_buffers(). > > It also fails to fix the other xfs_buf_resubmit_failed_buffers() > caller, which has exactly the same problem (dquot writeback). > Hrm I thought it was weird cscope showed only one caller, turns out my cscope.db was messed up from all the switching between branches. > Perhaps something like the patch below? > I thought about this, but I was worried that clearing the XFS_LI_FAILED may race with submitting the IO and having it fail again, so we end up clearing it when we need it set to resubmit again. But you are the expert here, if that isn't possible then I'm happy with this patch. Thanks, Josef
On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > > on the li, which clears the LI_FAILED flag, but also drops the ref on > > the buf. Since it isn't on a IO list at this point this could very well > > be the last ref on the buf, which wreaks havoc when we go to add the buf > > to the delwrite list. Fix this by holding a ref on the buf before we > > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > > doesn't disappear before we're able to clear the error and add it to the > > delwri list. This fixes the panics I was seeing with error injection. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/xfs/xfs_inode_item.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > > index fa1c4fe2ffbf..df49adf1989c 100644 > > --- a/fs/xfs/xfs_inode_item.c > > +++ b/fs/xfs/xfs_inode_item.c > > @@ -503,13 +503,16 @@ xfs_inode_item_push( > > * previously. Resubmit the buffer for IO. > > */ > > if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { > > - if (!xfs_buf_trylock(bp)) > > + xfs_buf_hold(bp); > > + if (!xfs_buf_trylock(bp)) { > > + xfs_buf_rele(bp); > > return XFS_ITEM_LOCKED; > > + } > > > > if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) > > rval = XFS_ITEM_FLUSHING; > > > > - xfs_buf_unlock(bp); > > + xfs_buf_relse(bp); > > return rval; > > } > > This just doesn't smell right to me. > > /me rummages around in the code > > Ok, so I think the bug is in xfs_buf_resubmit_failed_buffers() in > that it removes all the "failed" reference counts from the buffer > before it adds the delwri reference count back to the buffer. Taking > a high level reference count like above just papers over the > transient reference counting error in > xfs_buf_resubmit_failed_buffers(). > > It also fails to fix the other xfs_buf_resubmit_failed_buffers() > caller, which has exactly the same problem (dquot writeback). > > Perhaps something like the patch below? > The other question, is it possible for the buffer to be submitted in another thread immediately after it is queued for IO? Because then we have the same problem, we could get to xfs_buf_unlock() and it would have been freed in the other thread. If that's not possible either then I'm still cool with your patch. Thanks, Josef
On Thu, Nov 08, 2018 at 11:48:17AM +1100, Dave Chinner wrote: > [compendium reply] > > On Wed, Nov 07, 2018 at 06:43:03PM -0500, Josef Bacik wrote: > > On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > > > On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > > > > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > > > > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > > > > on the li, which clears the LI_FAILED flag, but also drops the ref on > > > > the buf. Since it isn't on a IO list at this point this could very well > > > > be the last ref on the buf, which wreaks havoc when we go to add the buf > > > > to the delwrite list. Fix this by holding a ref on the buf before we > > > > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > > > > doesn't disappear before we're able to clear the error and add it to the > > > > delwri list. This fixes the panics I was seeing with error injection. > .... > > > Perhaps something like the patch below? > > > > > > > I thought about this, but I was worried that clearing the XFS_LI_FAILED may race > > with submitting the IO and having it fail again, so we end up clearing it when > > we need it set to resubmit again. But you are the expert here, if that isn't > > possible then I'm happy with this patch. Thanks, > > The buffer cannot be submitted while we are clearing the failed > flags because a) the caller holds the buffer locked and so owns it > completely, and b) the caller owns the buffer_list that the buffer > is queued to and so controls when the list of buffers is submitted > for IO. > > IOWs, there is no possibility of racing with clearing the > XFS_LI_FAILED flags because we own everything in that context. > > > The other question, is it possible for the buffer to be submitted in another > > thread immediately after it is queued for IO? > > See a) above - you have to hold the buffer lock to submit it for IO. > Hence holding the buffer lock over queueing means nothing can submit > it for IO at the same time. And you have to hold the buffer lock to > submit it to the delwri list: > > > bool > xfs_buf_delwri_queue( > struct xfs_buf *bp, > struct list_head *list) > { > >>>>> ASSERT(xfs_buf_islocked(bp)); > ASSERT(!(bp->b_flags & XBF_READ)); > Ah yeah duh, thanks, Josef
On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > > on the li, which clears the LI_FAILED flag, but also drops the ref on > > the buf. Since it isn't on a IO list at this point this could very well > > be the last ref on the buf, which wreaks havoc when we go to add the buf > > to the delwrite list. Fix this by holding a ref on the buf before we > > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > > doesn't disappear before we're able to clear the error and add it to the > > delwri list. This fixes the panics I was seeing with error injection. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/xfs/xfs_inode_item.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > > index fa1c4fe2ffbf..df49adf1989c 100644 > > --- a/fs/xfs/xfs_inode_item.c > > +++ b/fs/xfs/xfs_inode_item.c > > @@ -503,13 +503,16 @@ xfs_inode_item_push( > > * previously. Resubmit the buffer for IO. > > */ > > if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { > > - if (!xfs_buf_trylock(bp)) > > + xfs_buf_hold(bp); > > + if (!xfs_buf_trylock(bp)) { > > + xfs_buf_rele(bp); > > return XFS_ITEM_LOCKED; > > + } > > > > if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) > > rval = XFS_ITEM_FLUSHING; > > > > - xfs_buf_unlock(bp); > > + xfs_buf_relse(bp); > > return rval; > > } > > This just doesn't smell right to me. > > /me rummages around in the code > > Ok, so I think the bug is in xfs_buf_resubmit_failed_buffers() in > that it removes all the "failed" reference counts from the buffer > before it adds the delwri reference count back to the buffer. Taking > a high level reference count like above just papers over the > transient reference counting error in > xfs_buf_resubmit_failed_buffers(). > > It also fails to fix the other xfs_buf_resubmit_failed_buffers() > caller, which has exactly the same problem (dquot writeback). > > Perhaps something like the patch below? > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > > > xfs: fix transient reference count error in xfs_buf_resubmit_failed_buffers > > From: Dave Chinner <dchinner@redhat.com> > > When retrying a failed inode or dquot buffer, > xfs_buf_resubmit_failed_buffers() clears all the failed flags from > the inode/dquot log items. In doing so, it also drops all the > reference counts on the buffer that the failed log items hold. This > means it can drop all the active references on the buffer and hence > free the buffer before it queues it for write again. > > Putting the buffer on the delwri queue takes a reference to the > buffer (so that it hangs around until it has been written and > completed), but this goes bang if the buffer has already been freed. > > Hence we need to add the buffer to the delwri queue before we remove > the failed flags from the log items attached to the buffer to ensure > it always remains referenced during the resubmit process. > > Reported-by: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Dave Chinner <dchinner@redhat.com> You can add Tested-by as well, this survives everything (ufortunately still no hang, so it's back to the drawing board for that.) Thanks, Josef
On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > > on the li, which clears the LI_FAILED flag, but also drops the ref on > > the buf. Since it isn't on a IO list at this point this could very well > > be the last ref on the buf, which wreaks havoc when we go to add the buf > > to the delwrite list. Fix this by holding a ref on the buf before we > > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > > doesn't disappear before we're able to clear the error and add it to the > > delwri list. This fixes the panics I was seeing with error injection. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/xfs/xfs_inode_item.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > > index fa1c4fe2ffbf..df49adf1989c 100644 > > --- a/fs/xfs/xfs_inode_item.c > > +++ b/fs/xfs/xfs_inode_item.c > > @@ -503,13 +503,16 @@ xfs_inode_item_push( > > * previously. Resubmit the buffer for IO. > > */ > > if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { > > - if (!xfs_buf_trylock(bp)) > > + xfs_buf_hold(bp); > > + if (!xfs_buf_trylock(bp)) { > > + xfs_buf_rele(bp); > > return XFS_ITEM_LOCKED; > > + } > > > > if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) > > rval = XFS_ITEM_FLUSHING; > > > > - xfs_buf_unlock(bp); > > + xfs_buf_relse(bp); > > return rval; > > } > > This just doesn't smell right to me. > > /me rummages around in the code > > Ok, so I think the bug is in xfs_buf_resubmit_failed_buffers() in > that it removes all the "failed" reference counts from the buffer > before it adds the delwri reference count back to the buffer. Taking > a high level reference count like above just papers over the > transient reference counting error in > xfs_buf_resubmit_failed_buffers(). > > It also fails to fix the other xfs_buf_resubmit_failed_buffers() > caller, which has exactly the same problem (dquot writeback). > > Perhaps something like the patch below? > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > > > xfs: fix transient reference count error in xfs_buf_resubmit_failed_buffers > > From: Dave Chinner <dchinner@redhat.com> > > When retrying a failed inode or dquot buffer, > xfs_buf_resubmit_failed_buffers() clears all the failed flags from > the inode/dquot log items. In doing so, it also drops all the > reference counts on the buffer that the failed log items hold. This > means it can drop all the active references on the buffer and hence > free the buffer before it queues it for write again. > > Putting the buffer on the delwri queue takes a reference to the > buffer (so that it hangs around until it has been written and > completed), but this goes bang if the buffer has already been freed. > > Hence we need to add the buffer to the delwri queue before we remove > the failed flags from the log items attached to the buffer to ensure > it always remains referenced during the resubmit process. > > Reported-by: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Dave Chinner <dchinner@redhat.com> Dave, Are you planning on sending this along as is? I'm going to throw it in our kernel if you are happy with it. Thanks, Josef
On Mon, Nov 12, 2018 at 09:23:48AM -0500, Josef Bacik wrote: > On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > > On Wed, Nov 07, 2018 at 03:10:55PM -0500, Josef Bacik wrote: > > > If we failed to writeout a xfs_buf we'll grab a ref for it and put it on > > > li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED > > > on the li, which clears the LI_FAILED flag, but also drops the ref on > > > the buf. Since it isn't on a IO list at this point this could very well > > > be the last ref on the buf, which wreaks havoc when we go to add the buf > > > to the delwrite list. Fix this by holding a ref on the buf before we > > > call xfs_buf_resubmit_failed_buffers in order to make sure the buf > > > doesn't disappear before we're able to clear the error and add it to the > > > delwri list. This fixes the panics I was seeing with error injection. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > --- > > > fs/xfs/xfs_inode_item.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > > > index fa1c4fe2ffbf..df49adf1989c 100644 > > > --- a/fs/xfs/xfs_inode_item.c > > > +++ b/fs/xfs/xfs_inode_item.c > > > @@ -503,13 +503,16 @@ xfs_inode_item_push( > > > * previously. Resubmit the buffer for IO. > > > */ > > > if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { > > > - if (!xfs_buf_trylock(bp)) > > > + xfs_buf_hold(bp); > > > + if (!xfs_buf_trylock(bp)) { > > > + xfs_buf_rele(bp); > > > return XFS_ITEM_LOCKED; > > > + } > > > > > > if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) > > > rval = XFS_ITEM_FLUSHING; > > > > > > - xfs_buf_unlock(bp); > > > + xfs_buf_relse(bp); > > > return rval; > > > } > > > > This just doesn't smell right to me. > > > > /me rummages around in the code > > > > Ok, so I think the bug is in xfs_buf_resubmit_failed_buffers() in > > that it removes all the "failed" reference counts from the buffer > > before it adds the delwri reference count back to the buffer. Taking > > a high level reference count like above just papers over the > > transient reference counting error in > > xfs_buf_resubmit_failed_buffers(). > > > > It also fails to fix the other xfs_buf_resubmit_failed_buffers() > > caller, which has exactly the same problem (dquot writeback). > > > > Perhaps something like the patch below? > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com > > > > > > xfs: fix transient reference count error in xfs_buf_resubmit_failed_buffers > > > > From: Dave Chinner <dchinner@redhat.com> > > > > When retrying a failed inode or dquot buffer, > > xfs_buf_resubmit_failed_buffers() clears all the failed flags from > > the inode/dquot log items. In doing so, it also drops all the > > reference counts on the buffer that the failed log items hold. This > > means it can drop all the active references on the buffer and hence > > free the buffer before it queues it for write again. > > > > Putting the buffer on the delwri queue takes a reference to the > > buffer (so that it hangs around until it has been written and > > completed), but this goes bang if the buffer has already been freed. > > > > Hence we need to add the buffer to the delwri queue before we remove > > the failed flags from the log items attached to the buffer to ensure > > it always remains referenced during the resubmit process. > > > > Reported-by: Josef Bacik <josef@toxicpanda.com> > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Dave, > > Are you planning on sending this along as is? I'm going to throw it in our > kernel if you are happy with it. Thanks, I'd appreciate it if this patch could be sent to the mailing list in its own thread to avoid "new patch buried in other thread" syndrome. --D > Josef
On Mon, Nov 12, 2018 at 09:23:48AM -0500, Josef Bacik wrote: > On Thu, Nov 08, 2018 at 10:37:40AM +1100, Dave Chinner wrote: > > xfs: fix transient reference count error in xfs_buf_resubmit_failed_buffers > > > > From: Dave Chinner <dchinner@redhat.com> > > > > When retrying a failed inode or dquot buffer, > > xfs_buf_resubmit_failed_buffers() clears all the failed flags from > > the inode/dquot log items. In doing so, it also drops all the > > reference counts on the buffer that the failed log items hold. This > > means it can drop all the active references on the buffer and hence > > free the buffer before it queues it for write again. > > > > Putting the buffer on the delwri queue takes a reference to the > > buffer (so that it hangs around until it has been written and > > completed), but this goes bang if the buffer has already been freed. > > > > Hence we need to add the buffer to the delwri queue before we remove > > the failed flags from the log items attached to the buffer to ensure > > it always remains referenced during the resubmit process. > > > > Reported-by: Josef Bacik <josef@toxicpanda.com> > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Dave, > > Are you planning on sending this along as is? I'm going to throw it in our > kernel if you are happy with it. Thanks, I'm planning to, it's just my stack of fixes is growing faster than I can QA them at the moment. Cheers, Dave.
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index fa1c4fe2ffbf..df49adf1989c 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -503,13 +503,16 @@ xfs_inode_item_push( * previously. Resubmit the buffer for IO. */ if (test_bit(XFS_LI_FAILED, &lip->li_flags)) { - if (!xfs_buf_trylock(bp)) + xfs_buf_hold(bp); + if (!xfs_buf_trylock(bp)) { + xfs_buf_rele(bp); return XFS_ITEM_LOCKED; + } if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list)) rval = XFS_ITEM_FLUSHING; - xfs_buf_unlock(bp); + xfs_buf_relse(bp); return rval; }
If we failed to writeout a xfs_buf we'll grab a ref for it and put it on li->li_buf. Then when submitting the failed bufs we'll clear LI_FAILED on the li, which clears the LI_FAILED flag, but also drops the ref on the buf. Since it isn't on a IO list at this point this could very well be the last ref on the buf, which wreaks havoc when we go to add the buf to the delwrite list. Fix this by holding a ref on the buf before we call xfs_buf_resubmit_failed_buffers in order to make sure the buf doesn't disappear before we're able to clear the error and add it to the delwri list. This fixes the panics I was seeing with error injection. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/xfs/xfs_inode_item.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)