xfs
[Top] [All Lists]

Re: [PATCH 05/11] xfs: use EFI refcount consistently in log recovery

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 05/11] xfs: use EFI refcount consistently in log recovery
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 7 Aug 2015 08:09:54 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150807015133.GE16638@dastard>
References: <1438883072-28706-1-git-send-email-bfoster@xxxxxxxxxx> <1438883072-28706-6-git-send-email-bfoster@xxxxxxxxxx> <20150807015133.GE16638@dastard>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Aug 07, 2015 at 11:51:33AM +1000, Dave Chinner wrote:
> On Thu, Aug 06, 2015 at 01:44:26PM -0400, Brian Foster wrote:
> > @@ -2933,16 +2933,16 @@ xlog_recover_efi_pass2(
> >     struct xlog_recover_item        *item,
> >     xfs_lsn_t                       lsn)
> >  {
> > -   int                     error;
> > -   xfs_mount_t             *mp = log->l_mp;
> > -   xfs_efi_log_item_t      *efip;
> > -   xfs_efi_log_format_t    *efi_formatp;
> > +   int                             error;
> > +   struct xfs_mount                *mp = log->l_mp;
> > +   struct xfs_efi_log_item         *efip;
> > +   struct xfs_efi_log_format       *efi_formatp;
> >  
> >     efi_formatp = item->ri_buf[0].i_addr;
> >  
> >     efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
> > -   if ((error = xfs_efi_copy_format(&(item->ri_buf[0]),
> > -                                    &(efip->efi_format)))) {
> > +   error = xfs_efi_copy_format(&(item->ri_buf[0]), &(efip->efi_format));
> > +   if (error) {
> 
> You can kill the extra (..) around the variables here.
> 

Ok.

> ....
> 
> > @@ -2995,18 +2998,18 @@ xlog_recover_efd_pass2(
> >                     efip = (xfs_efi_log_item_t *)lip;
> >                     if (efip->efi_format.efi_id == efi_id) {
> >                             /*
> > -                            * xfs_trans_ail_delete() drops the
> > -                            * AIL lock.
> > +                            * Drop the EFD reference to the EFI. This
> > +                            * removes the EFI from the AIL and frees it.
> >                              */
> > -                           xfs_trans_ail_delete(ailp, lip,
> > -                                                SHUTDOWN_CORRUPT_INCORE);
> > -                           xfs_efi_item_free(efip);
> > +                           spin_unlock(&ailp->xa_lock);
> > +                           xfs_efi_release(efip);
> >                             spin_lock(&ailp->xa_lock);
> 
> Need to call xfs_efi_release() outside the ailp->xa_lock as
> xfs_efi_release() now does the removal of the log item from the AIL
> and so can deadlock when taking the ailp->xa_lock.
> 

It drops and reacquires the lock, hence the removal of the comment about
xfs_trans_ail_delete() dropping it. ;)

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>