xfs
[Top] [All Lists]

Re: [PATCH 2/5] xfs: use generic percpu counters for inode counter

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: use generic percpu counters for inode counter
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 3 Feb 2015 06:33:44 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150202164409.GA695@xxxxxxxxxxxxx>
References: <1422826983-29570-1-git-send-email-david@xxxxxxxxxxxxx> <1422826983-29570-3-git-send-email-david@xxxxxxxxxxxxx> <20150202164409.GA695@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Feb 02, 2015 at 08:44:09AM -0800, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 4cf335b..7bfa527 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -357,7 +357,8 @@ __xfs_sb_from_disk(
> >     to->sb_rextslog = from->sb_rextslog;
> >     to->sb_inprogress = from->sb_inprogress;
> >     to->sb_imax_pct = from->sb_imax_pct;
> > -   to->sb_icount = be64_to_cpu(from->sb_icount);
> > +   if (percpu_counter_initialized(&to->sb_icount))
> > +           percpu_counter_set(&to->sb_icount, 
> > be64_to_cpu(from->sb_icount));
> 
> Why would the percpu counter not be initialized here?  Oh, I guess
> this is for xfs_sb_verify().  But why can't xfs_mount_validate_sb simply
> operate on the disk endian SB to avoid that whole issue?

Possibly. I'll look into it.

> > @@ -1288,8 +1288,11 @@ xfs_mod_incore_sb(
> >     int                     status;
> >  
> >  #ifdef HAVE_PERCPU_SB
> > -   ASSERT(field < XFS_SBS_ICOUNT || field > XFS_SBS_FDBLOCKS);
> > +   ASSERT(field < XFS_SBS_IFREE || field > XFS_SBS_FDBLOCKS);
> >  #endif
> > +   if (field == XFS_SBS_ICOUNT)
> > +           return xfs_mod_incore_sb_unlocked(mp, field, delta, rsvd);
> > +
> 
> Why is this multiplexd through xfs_mod_incore_sb_unlocked while needing
> a different locking context?  Shouldn't we simply use a different helper
> for this case?

Again, expedient. To fix, I need to export
xfs_mod_incore_sb_unlocked().

> >     xfs_icsb_cnts_t *cntp;
> >     int             i;
> >  
> > +   i = percpu_counter_init(&mp->m_sb.sb_icount, 0, GFP_KERNEL);
> > +   if (i)
> > +           return ENOMEM;
> > +
> >     mp->m_sb_cnts = alloc_percpu(xfs_icsb_cnts_t);
> > -   if (mp->m_sb_cnts == NULL)
> > +   if (!mp->m_sb_cnts) {
> > +           percpu_counter_destroy(&mp->m_sb.sb_icount);
> >             return -ENOMEM;
> > +   }
> >  
> >     for_each_online_cpu(i) {
> 
> Reusing a variable for both an errno value and a loop iterator is
> not very readable, just add an additional "error" variabe.

In the end it gets renamed to error. I'll fix it up.

> Also percpu_counter_init returns a proper egative errno value, no need
> to turn that into the incorrect postive ENOMEM.

Oversight. Will fix.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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