xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: use generic percpu counters for inode counter
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 2 Feb 2015 08:44:09 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1422826983-29570-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1422826983-29570-1-git-send-email-david@xxxxxxxxxxxxx> <1422826983-29570-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
> 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?

> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 6015f54..df5ec55 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1127,13 +1127,13 @@ xfs_mod_incore_sb_unlocked(
>        */
>       switch (field) {
>       case XFS_SBS_ICOUNT:
> +             /* deltas are +/-64, hence the large batch size of 128. */
> +             __percpu_counter_add(&mp->m_sb.sb_icount, delta, 128);
> +             if (percpu_counter_compare(&mp->m_sb.sb_icount, 0) < 0) {
>                       ASSERT(0);
> +                     percpu_counter_add(&mp->m_sb.sb_icount, -delta);
>                       return -EINVAL;
>               }
>               return 0;
>       case XFS_SBS_IFREE:
>               lcounter = (long long)mp->m_sb.sb_ifree;
> @@ -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?

>       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.

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

Additionally should this use goto unwining?

>       if (idelta) {
> -             error = xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT,
> -                                              idelta, rsvd);
> +             error = xfs_mod_incore_sb(mp, XFS_SBS_ICOUNT, idelta, rsvd);

Why go through xfs_mod_incore_sb here instead of directly jumping to
the function that does the work?

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