Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Wed, 13 Sep 2023 16:49:52 +0200
From:   David Sterba <dsterba@suse.cz>
To:     Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
Cc:     "dsterba@suse.cz" <dsterba@suse.cz>, Chris Mason <clm@fb.com>,
        Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>,
        Christoph Hellwig <hch@lst.de>,
        Naohiro Aota <Naohiro.Aota@wdc.com>, Qu Wenruo <wqu@suse.com>,
        Damien Le Moal <dlemoal@kernel.org>,
        "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 01/11] btrfs: add raid stripe tree definitions
Message-ID: <20230913144951.GL20408@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20230911-raid-stripe-tree-v8-0-647676fa852c@wdc.com>
 <20230911-raid-stripe-tree-v8-1-647676fa852c@wdc.com>
 <20230912203214.GE20408@twin.jikos.cz>
 <50cfa5a0-c209-430f-8c00-54ba41c3791d@wdc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <50cfa5a0-c209-430f-8c00-54ba41c3791d@wdc.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Wed, Sep 13, 2023 at 06:02:09AM +0000, Johannes Thumshirn wrote:
> On 12.09.23 22:32, David Sterba wrote:
> >> @@ -306,6 +306,16 @@ BTRFS_SETGET_FUNCS(timespec_nsec, struct btrfs_timespec, nsec, 32);
> >>   BTRFS_SETGET_STACK_FUNCS(stack_timespec_sec, struct btrfs_timespec, sec, 64);
> >>   BTRFS_SETGET_STACK_FUNCS(stack_timespec_nsec, struct btrfs_timespec, nsec, 32);
> >>   
> >> +BTRFS_SETGET_FUNCS(stripe_extent_encoding, struct btrfs_stripe_extent, encoding, 8);
> > 
> > What is encoding referring to?
> 
> At the moment (only) the RAID type. But in the future it can be expanded 
> to all kinds of encodings, like Reed-Solomon, Butterfly-Codes, etc...

I see, could it be better called ECC? Like stripe_extent_ecc, that would
be clear that it's for the correction, encoding sounds is too generic.

> >> +	__le64 devid;
> >> +	/* physical location on disk */
> >> +	__le64 physical;
> >> +	/* length of stride on this disk */
> >> +	__le64 length;
> >> +};
> > 
> > __attribute__ ((__packed__));
> 
> The structure doesn't have any holes in it so packed is not needed.
> 
> I might also be misinformed, but doesn't packed potentially lead to bad 
> code generation on some platforms?  I've always been under the 
> impression that packed forces the compiler to do byte-wise loads and 
> stores. But as I've said, I might be misinformed.

All on-disk structures have the packed attribute so for consistency and
future safety it should be here too, even if it technically does not
need it due to alignment. In addition, strucutres that need padding
would be also problematic, e.g. u64 followed by u32 needs 4 bytes of
padding but the next item after it would be placed right after u32.

It's right that on some platforms unaligned access is done by more code
but for the same reason on such platforms we can't let the compiler
decide the layout when the structure is directly mapped onto the blocks.