Page MenuHomeFreeBSD

Extend stripeoffset and stripesize of GEOMs from u_int to off_t
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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.

sys/geom/geom_disk.h
100 ↗(On Diff #36356)

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 ↗(On Diff #36356)

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 ↗(On Diff #36356)

I do not like the gap between sectorsize and stripesize.

eugen_grosbein.net added inline comments.
sys/geom/geom_disk.h
100 ↗(On Diff #36356)

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 added inline comments.
sys/geom/geom.h
204 ↗(On Diff #36356)

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 test plan for this revision. (Show Details)
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.

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#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:

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).

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

All changes are within kernel land only and ioctls DIOCGSTRIPE{SIZE|OFFSET} already use off_t.

I'm going to commit this soon unless an objection is raised.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 27 2018, 4:14 PM
This revision was automatically updated to reflect the committed changes.