LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Kent Overstreet <kent.overstreet@gmail.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-bcache@vger.kernel.org, Dave Chinner <dchinner@redhat.com>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	hare@suse.com
Subject: Re: [PATCH 1/2] bcachefs: On disk data structures
Date: Fri, 11 May 2018 18:32:33 +1000	[thread overview]
Message-ID: <20180511083233.GX23861@dastard> (raw)
In-Reply-To: <20180508221800.2642-2-kent.overstreet@gmail.com>

Hi Kent,

I haven't really had time to digest this in any real detail,
but I've noticed a couple of things that worry me...

On Tue, May 08, 2018 at 06:17:59PM -0400, Kent Overstreet wrote:
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> ---
>  fs/bcachefs/bcachefs_format.h | 1448 +++++++++++++++++++++++++++++++++
>  1 file changed, 1448 insertions(+)
>  create mode 100644 fs/bcachefs/bcachefs_format.h
> 
> diff --git a/fs/bcachefs/bcachefs_format.h b/fs/bcachefs/bcachefs_format.h
> new file mode 100644
> index 0000000000..0961585c7e
> --- /dev/null
> +++ b/fs/bcachefs/bcachefs_format.h
> @@ -0,0 +1,1448 @@
> +#ifndef _BCACHEFS_FORMAT_H
> +#define _BCACHEFS_FORMAT_H
.....
> +/* Btree keys - all units are in sectors */
> +
> +struct bpos {
> +	/* Word order matches machine byte order */
> +#if defined(__LITTLE_ENDIAN)
> +	__u32		snapshot;
> +	__u64		offset;
> +	__u64		inode;
> +#elif defined(__BIG_ENDIAN)
> +	__u64		inode;
> +	__u64		offset;		/* Points to end of extent - sectors */
> +	__u32		snapshot;
> +#else

Mostly my concerns are about these endian constructs - is the on
disk structure big endian or little endian, and how do you ensure
that everything you read and write to the on-disk format is in the
correct endian notation? I think your on-disk format is little
endian (from the definitions later in the file) but these don't look
like endian neutral structures....

That's apart from the fact all the endian defines make the code
really hard to read, and probably a pain to maintain, and it doubles
the test matrix because any on-disk change has to be validate on
both little endian and big endian machines....

> +union bch_extent_entry {
> +#if defined(__LITTLE_ENDIAN) ||  __BITS_PER_LONG == 64
> +	unsigned long			type;
> +#elif __BITS_PER_LONG == 32
> +	struct {
> +		unsigned long		pad;
> +		unsigned long		type;
> +	};
> +#else

This is another worry - using "long" in the on disk structure
definition. If this is in-meory structures, then use
le64_to_cpu/cpu_to_le64 to convert the value from the on-disk value
to the in-memory, cpu order value....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2018-05-11  8:32 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08 22:17 [PATCH 0/2] bcachefs: on disk data structures, ioctl interface - review requested for upstreaming Kent Overstreet
2018-05-08 22:17 ` [PATCH 1/2] bcachefs: On disk data structures Kent Overstreet
2018-05-11  8:32   ` Dave Chinner [this message]
2018-05-11 22:04     ` Kent Overstreet
2018-05-13 20:30   ` Randy Dunlap
2018-05-13 22:29     ` Kent Overstreet
2018-05-13 22:49       ` Randy Dunlap
2018-05-08 22:18 ` [PATCH 2/2] bcachefs: Ioctl interface Kent Overstreet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180511083233.GX23861@dastard \
    --to=david@fromorbit.com \
    --cc=darrick.wong@oracle.com \
    --cc=dchinner@redhat.com \
    --cc=hare@suse.com \
    --cc=kent.overstreet@gmail.com \
    --cc=linux-bcache@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 1/2] bcachefs: On disk data structures' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).