xfs
[Top] [All Lists]

Re: [PATCH 2/9] xfs: Pull EFI/EFD handling out from under the AIL lock

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/9] xfs: Pull EFI/EFD handling out from under the AIL lock
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 20 Dec 2010 12:00:10 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101217112254.GB12965@xxxxxxxxxxxxx>
References: <1292214743-18073-1-git-send-email-david@xxxxxxxxxxxxx> <1292214743-18073-3-git-send-email-david@xxxxxxxxxxxxx> <20101217112254.GB12965@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Dec 17, 2010 at 06:22:54AM -0500, Christoph Hellwig wrote:
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> 
> Some minor comments below:
> 
> > +STATIC void
> > +__xfs_efi_release(
> > +   struct xfs_efi_log_item *efip)
> > +{
> > +   struct xfs_ail          *ailp = efip->efi_item.li_ailp;
> > +
> > +   if (!test_and_clear_bit(XFS_EFI_COMMITTED, &efip->efi_flags)) {
> > +           spin_lock(&ailp->xa_lock);
> > +           /* xfs_trans_ail_delete() drops the AIL lock. */
> > +           xfs_trans_ail_delete(ailp, (xfs_log_item_t *)efip);
> 
> The second argument should be &efip->efi_item to preserve ty;e safety.
> 
> >  void
> >  xfs_efi_release(xfs_efi_log_item_t *efip,
> >             uint                    nextents)
> >  {
> > +   ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
> > +   if (!atomic_sub_and_test(nextents, &efip->efi_next_extent))
> > +           return;
> >  
> > +   __xfs_efi_release(efip);
> 
> Why not just:
> 
>       if (atomic_sub_and_test(nextents, &efip->efi_next_extent))
>               __xfs_efi_release(efip);
> 
> ?

Good idea. Fixed up.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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