Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

From: David Sterba <dsterba@suse.cz>
To: Liu Bo <bo.li.liu@oracle.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work
Date: Fri, 4 Dec 2015 13:36:25 +0100	[thread overview]
Message-ID: <20151204123625.GF31035@suse.cz> (raw)
In-Reply-To: <20151204022537.GH19589@localhost.localdomain>

On Thu, Dec 03, 2015 at 06:25:37PM -0800, Liu Bo wrote:
> >  	struct inode *inode;
> > -	int delay_iput;
> >  	struct completion completion;
> >  	struct list_head list;
> >  	struct btrfs_work work;
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 15b29e879ffc..529a53b80ca0 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -9436,16 +9436,21 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
> >  {
> >  	struct btrfs_delalloc_work *delalloc_work;
> >  	struct inode *inode;
> > +	int delay_iput;
> >  
> >  	delalloc_work = container_of(work, struct btrfs_delalloc_work,
> >  				     work);
> >  	inode = delalloc_work->inode;
> > +	/* Lowest bit of inode pointer tracks the delayed status */
> > +	delay_iput = ((unsigned long)inode & 1UL);
> > +	inode = (struct inode *)((unsigned long)inode & ~1UL);
> > +
> 
> To be quite frankly, I don't like this, it's a pointer anyway,
> error-prone in a debugging view, instead would 'u8 delayed_iput' help?

The point was to shrink the structure. Adding the u8 will grow it by
another 8 bytes, besides the slab objects are aligned to 8 bytes by
default so the overall cost of storing the delayed information is 8
bytes:

struct btrfs_delalloc_work {
        struct inode *             inode;                /*     0     8 */
        struct completion          completion;           /*     8    32 */
        struct list_head           list;                 /*    40    16 */
        struct btrfs_work          work;                 /*    56    88 */
        /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */
        u8                         delay;                /*   144     1 */

        /* size: 152, cachelines: 3, members: 5 */
        /* padding: 7 */
        /* last cacheline: 24 bytes */
};

As the use of the inode pointer is limited, I don't think this would
cause surprises. And it's commented where used which should help during
debugging.

Abusing the low bits of pointers is nothing new, the page cache tags are
implemented that way. This kind of low-level optimizations is IMO acceptable.