| Message ID | 20200710140511.30343-2-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Two furhter additions for fsinfo ioctl | expand |
On 10/7/20 10:05 pm, Johannes Thumshirn wrote:
> + __u32 generation; /* out */
Generation is defined as u64 in fs_info.
struct btrfs_fs_info {
u64 generation;
Thanks, Anand
On 10/07/2020 16:32, Anand Jain wrote: > On 10/7/20 10:05 pm, Johannes Thumshirn wrote: >> + __u32 generation; /* out */ > > Generation is defined as u64 in fs_info. > > struct btrfs_fs_info { > > u64 generation; *doh* Thanks for spotting
On Fri, Jul 10, 2020 at 11:05:09PM +0900, Johannes Thumshirn wrote: > @@ -261,7 +264,8 @@ struct btrfs_ioctl_fs_info_args { > __u32 flags; /* in/out */ > __u16 csum_type; /* out */ > __u16 csum_size; /* out */ > - __u8 reserved[972]; /* pad to 1k */ > + __u32 generation; /* out */ > + __u8 reserved[968]; /* pad to 1k */ I've tested the static assert by switching just the type but not the remaining reserved bytes ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_fs_info_args) == 1024" 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~~~~~~~~~~~ ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ 77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) | ^~~~~~~~~~~~~~~ ./include/uapi/linux/btrfs.h:270:1: note: in expansion of macro ‘static_assert’ 270 | static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024); | ^~~~~~~~~~~~~ make[2]: *** [scripts/Makefile.build:281: fs/btrfs/super.o] Error 1 make[1]: *** [scripts/Makefile.build:497: fs/btrfs] Error 2 make: *** [Makefile:1756: fs] Error 2 Good.
On Mon, Jul 13, 2020 at 11:42:51AM +0200, David Sterba wrote: > On Fri, Jul 10, 2020 at 11:05:09PM +0900, Johannes Thumshirn wrote: > > @@ -261,7 +264,8 @@ struct btrfs_ioctl_fs_info_args { > > __u32 flags; /* in/out */ > > __u16 csum_type; /* out */ > > __u16 csum_size; /* out */ > > - __u8 reserved[972]; /* pad to 1k */ > > + __u32 generation; /* out */ > > + __u8 reserved[968]; /* pad to 1k */ > > I've tested the static assert by switching just the type but not the > remaining reserved bytes > > ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_fs_info_args) == 1024" > 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) > | ^~~~~~~~~~~~~~ > ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ > 77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) > | ^~~~~~~~~~~~~~~ > ./include/uapi/linux/btrfs.h:270:1: note: in expansion of macro ‘static_assert’ > 270 | static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024); > | ^~~~~~~~~~~~~ > make[2]: *** [scripts/Makefile.build:281: fs/btrfs/super.o] Error 1 > make[1]: *** [scripts/Makefile.build:497: fs/btrfs] Error 2 > make: *** [Makefile:1756: fs] Error 2 > > Good. There's extra padding now required for u64: struct btrfs_ioctl_fs_info_args { __u64 max_id; /* 0 8 */ __u64 num_devices; /* 8 8 */ __u8 fsid[16]; /* 16 16 */ __u32 nodesize; /* 32 4 */ __u32 sectorsize; /* 36 4 */ __u32 clone_alignment; /* 40 4 */ __u32 flags; /* 44 4 */ __u16 csum_type; /* 48 2 */ __u16 csum_size; /* 50 2 */ /* XXX 4 bytes hole, try to pack */ __u64 generation; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ __u8 reserved[964]; /* 64 964 */ /* size: 1032, cachelines: 17, members: 11 */ /* sum members: 1024, holes: 1, sum holes: 4 */ /* padding: 4 */ /* last cacheline: 8 bytes */ }; What if, instead of inserting a padding/reserved field we switch the flags to u64 too. This unfortunatelly requires swapping order for flags and csum_type/_size but the result struct btrfs_ioctl_fs_info_args { __u64 max_id; /* 0 8 */ __u64 num_devices; /* 8 8 */ __u8 fsid[16]; /* 16 16 */ __u32 nodesize; /* 32 4 */ __u32 sectorsize; /* 36 4 */ __u32 clone_alignment; /* 40 4 */ __u16 csum_type; /* 44 2 */ __u16 csum_size; /* 46 2 */ __u64 flags; /* 48 8 */ __u64 generation; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ __u8 reserved[960]; /* 64 960 */ /* size: 1024, cachelines: 16, members: 11 */ }; does not require any padding and leaves the end member with 8 byte alignment.
On 13/07/2020 11:53, David Sterba wrote: > On Mon, Jul 13, 2020 at 11:42:51AM +0200, David Sterba wrote: >> On Fri, Jul 10, 2020 at 11:05:09PM +0900, Johannes Thumshirn wrote: >>> @@ -261,7 +264,8 @@ struct btrfs_ioctl_fs_info_args { >>> __u32 flags; /* in/out */ >>> __u16 csum_type; /* out */ >>> __u16 csum_size; /* out */ >>> - __u8 reserved[972]; /* pad to 1k */ >>> + __u32 generation; /* out */ >>> + __u8 reserved[968]; /* pad to 1k */ >> >> I've tested the static assert by switching just the type but not the >> remaining reserved bytes >> >> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_fs_info_args) == 1024" >> 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) >> | ^~~~~~~~~~~~~~ >> ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ >> 77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) >> | ^~~~~~~~~~~~~~~ >> ./include/uapi/linux/btrfs.h:270:1: note: in expansion of macro ‘static_assert’ >> 270 | static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024); >> | ^~~~~~~~~~~~~ >> make[2]: *** [scripts/Makefile.build:281: fs/btrfs/super.o] Error 1 >> make[1]: *** [scripts/Makefile.build:497: fs/btrfs] Error 2 >> make: *** [Makefile:1756: fs] Error 2 >> >> Good. > > There's extra padding now required for u64: > > struct btrfs_ioctl_fs_info_args { > __u64 max_id; /* 0 8 */ > __u64 num_devices; /* 8 8 */ > __u8 fsid[16]; /* 16 16 */ > __u32 nodesize; /* 32 4 */ > __u32 sectorsize; /* 36 4 */ > __u32 clone_alignment; /* 40 4 */ > __u32 flags; /* 44 4 */ > __u16 csum_type; /* 48 2 */ > __u16 csum_size; /* 50 2 */ > > /* XXX 4 bytes hole, try to pack */ > > __u64 generation; /* 56 8 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > __u8 reserved[964]; /* 64 964 */ > > /* size: 1032, cachelines: 17, members: 11 */ > /* sum members: 1024, holes: 1, sum holes: 4 */ > /* padding: 4 */ > /* last cacheline: 8 bytes */ > }; > > What if, instead of inserting a padding/reserved field we switch the > flags to u64 too. This unfortunatelly requires swapping order for flags > and csum_type/_size but the result > > struct btrfs_ioctl_fs_info_args { > __u64 max_id; /* 0 8 */ > __u64 num_devices; /* 8 8 */ > __u8 fsid[16]; /* 16 16 */ > __u32 nodesize; /* 32 4 */ > __u32 sectorsize; /* 36 4 */ > __u32 clone_alignment; /* 40 4 */ > __u16 csum_type; /* 44 2 */ > __u16 csum_size; /* 46 2 */ > __u64 flags; /* 48 8 */ > __u64 generation; /* 56 8 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > __u8 reserved[960]; /* 64 960 */ > > /* size: 1024, cachelines: 16, members: 11 */ > }; > > does not require any padding and leaves the end member with 8 byte > alignment. > The swapped order looks a bit odd, but I don't really see a way around it. Can you fix that up or should I re-send all 4 patches?
On Mon, Jul 13, 2020 at 09:57:15AM +0000, Johannes Thumshirn wrote: > On 13/07/2020 11:53, David Sterba wrote: > > On Mon, Jul 13, 2020 at 11:42:51AM +0200, David Sterba wrote: > >> On Fri, Jul 10, 2020 at 11:05:09PM +0900, Johannes Thumshirn wrote: > > struct btrfs_ioctl_fs_info_args { > > __u64 max_id; /* 0 8 */ > > __u64 num_devices; /* 8 8 */ > > __u8 fsid[16]; /* 16 16 */ > > __u32 nodesize; /* 32 4 */ > > __u32 sectorsize; /* 36 4 */ > > __u32 clone_alignment; /* 40 4 */ > > __u16 csum_type; /* 44 2 */ > > __u16 csum_size; /* 46 2 */ > > __u64 flags; /* 48 8 */ > > __u64 generation; /* 56 8 */ > > /* --- cacheline 1 boundary (64 bytes) --- */ > > __u8 reserved[960]; /* 64 960 */ > > > > /* size: 1024, cachelines: 16, members: 11 */ > > }; > > > > does not require any padding and leaves the end member with 8 byte > > alignment. > > The swapped order looks a bit odd, but I don't really see a way around it. > Can you fix that up or should I re-send all 4 patches? Please resend all 4, I'll drop and replace the csum_* patch in misc-next. Thanks.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 3a566cf71fc6..f1b433ec09e8 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3247,6 +3247,11 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_INFO; } + if (flags_in & BTRFS_FS_INFO_FLAG_GENERATION) { + fi_args->generation = fs_info->generation; + fi_args->flags |= BTRFS_FS_INFO_FLAG_GENERATION; + } + if (copy_to_user(arg, fi_args, sizeof(*fi_args))) ret = -EFAULT; diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index 0f81f69dbe22..19609dd5db18 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -250,6 +250,9 @@ struct btrfs_ioctl_dev_info_args { /* Request information about checksum type and size */ #define BTRFS_FS_INFO_FLAG_CSUM_INFO (1 << 0) +/* Request information about filesystem generation */ +#define BTRFS_FS_INFO_FLAG_GENERATION (1 << 1) + struct btrfs_ioctl_fs_info_args { __u64 max_id; /* out */ __u64 num_devices; /* out */ @@ -261,7 +264,8 @@ struct btrfs_ioctl_fs_info_args { __u32 flags; /* in/out */ __u16 csum_type; /* out */ __u16 csum_size; /* out */ - __u8 reserved[972]; /* pad to 1k */ + __u32 generation; /* out */ + __u8 reserved[968]; /* pad to 1k */ };
Add retrieval of the filesystem's generation to the fsinfo ioctl. This is driven by setting the BTRFS_FS_INFO_FLAG_GENERATION flag in btrfs_ioctl_fs_info_args::flags. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/ioctl.c | 5 +++++ include/uapi/linux/btrfs.h | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-)