Extend stripeoffset and stripesize of GEOMs from u_int to off_t
ClosedPublic
Actions

Authored by eugen_grosbein.net on Dec 8 2017, 5:18 PM.

Details

Reviewers

imp
kib
allanjude
jhb
mav
avg
pjd

Commits

rS339815: Extend stripeoffset and stripesize of GEOMs from u_int to off_t

Summary

GEOM's stripeoffset overflows at 4 gigabyte margin (2^32) because of its u_int type. This leads to incorrect data in the output generated by "sysctl kern.geom.confxml" command, "graid list" etc. when GEOM array has volumes larger than 4G.

Test Plan

Create an array with first RAID1 volume /dev/raid/r0 spanning first 4097 megabytes of ada0 and ada1 disks:

$ graid label -S 4097M Promise r0 RAID1 ada0 ada1

Then, add second SINGLE volume /dev/raid/r1 to the same array utilizing the rest of ada0:

$ graid label Promise r1 SINGLE ada0
$ graid list | grep -A4 raid/r1
2. Name: raid/r1
   Mediasize: 6374293504 (5.9G)
   Sectorsize: 512
   Stripesize: 0
   Stripeoffset: 1048576

It shows stripeoffset as 1M instead of 4097M due to overflow in the GEOM despite of right 64 bit offsets within GEOM_RAID itself.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 13443

Event Timeline

eugen_grosbein.net created this revision.Dec 8 2017, 5:18 PM

eugen_grosbein.net edited the test plan for this revision. (Show Details)

eugen_grosbein.net edited the test plan for this revision. (Show Details)Dec 8 2017, 5:21 PM

eugen_grosbein.net added a reviewer: avg.

eugen_grosbein.net edited the summary of this revision. (Show Details)Dec 8 2017, 5:25 PM

As I have written in private: I see no big problem here, while applying this breaks an ABIs (BTW there should be bumped disk ABI version). But other thoughts are welcome.

imp added inline comments.Dec 8 2017, 6:17 PM

sys/geom/geom_disk.h
100	this breaks binary compat, so we need #define DISK_VERSION_06 0x5856105f #define DISK_VERSION DISK_VERSION_06 to make sure the breakage is enforced.
sys/geom/raid/g_raid.c
2426	surprised this didn't whine before.

Does this change break ABI or KBI ? Or both ?

Some providers take stripesize/offset in create, e.g. nop. Do they need a fix included into the change ?

sys/geom/geom.h
204	I do not like the gap between sectorsize and stripesize.

DISK_VERSION bumped.

eugen_grosbein.net marked an inline comment as done.Dec 8 2017, 8:36 PM

eugen_grosbein.net added inline comments.

sys/geom/geom_disk.h
100	Thanks! Done.

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

Some providers take stripesize/offset in create, e.g. nop. Do they need a fix included into the change ?

nop and others not mentioned in the diff use "off_t" internaly already.

eugen_grosbein.net marked an inline comment as done.Dec 8 2017, 8:47 PM

eugen_grosbein.net added inline comments.

sys/geom/geom.h
204	Do you mean that compiler will use 8-byte alignment ffo "stripesize", so "sectorsize" should be moved nead the end of the struct?

eugen_grosbein.net retitled this revision from Extend stripeoffset and stripesize of GEOMs from uint_t to off_t to Extend stripeoffset and stripesize of GEOMs from u_int to off_t.Dec 8 2017, 8:49 PM

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

It seems, yes. No MFC planned.

eugen_grosbein.net edited the summary of this revision. (Show Details)Dec 8 2017, 9:07 PM

eugen_grosbein.net edited the test plan for this revision. (Show Details)

In D13426#280601, @eugen_grosbein.net wrote:

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

It seems, yes. No MFC planned.

The 'yes' answer to tri-state question is not informative.

If there is ABI breakage, its impact must be evaluated. It could be that this change is not acceptable if affecting symver-ed interfaces.

In D13426#280595, @eugen_grosbein.net wrote:

nop and others not mentioned in the diff use "off_t" internaly already.

Look at the argument types for g_nop_create() as an example.

In D13426#280610, @kib wrote:

In D13426#280601, @eugen_grosbein.net wrote:

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

It seems, yes. No MFC planned.

The 'yes' answer to tri-state question is not informative.

If there is ABI breakage, its impact must be evaluated. It could be that this change is not acceptable if affecting symver-ed interfaces.

I believe this is a KBI breakage. I don't think it's an ABI breakage. But I don't see any changes to libgeom.

In D13426#280612, @imp wrote:

I believe this is a KBI breakage. I don't think it's an ABI breakage. But I don't see any changes to libgeom.

libgeom already uses off_t/ssize_t/strtoumax for stripesize/stripeoffset.

More changes: geom_nop, geom_stripe and geom_redboot.

In D13426#280611, @kib wrote:

In D13426#280595, @eugen_grosbein.net wrote:

nop and others not mentioned in the diff use "off_t" internaly already.

Look at the argument types for g_nop_create() as an example.

Yes, I've missed that part, thanks. More changes added for geom_nop and geom_stripe (but not its on-disk metadata), and for geom_redboot (not sure if it's connected to build, though).