Add ability to label md(4) devices
ClosedPublic

Authored by sobomax on Apr 22 2017, 3:50 AM.

Details

Summary

We rely on md(4) in our build process heavily. However, if the build goes haywire the allocated resources (i.e. swap and memory-backed md(4)'s) need to be purged. It is extremely useful to have ability to attach arbitrary labels to each of the virtual disks so that they can be identified and GC'ed if neecessary.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sobomax created this revision.Apr 22 2017, 3:50 AM
imp added a comment.Apr 22 2017, 3:55 AM

Love this idea, but the padding issue needs to be resolved.

sys/sys/mdioctl.h
52 ↗(On Diff #27626)

This changes the ABI on 64-bit architectures, since an int is only 32-bits there, but a pointer is 64 so the sizeof md_ioctl grows by 4 bytes.

In D10457#216855, @imp wrote:

Love this idea, but the padding issue needs to be resolved.

Hmm, are you sure? I think I've actually checked it goes down by 8 bytes. There is alignment, the structure is not packed, so that compiler I think aligns everything to the natural word size on 64-bits. At least on intel it's what's happening.

-Max

I've checked both forward and backward compatibility (3-way actually, old mdconfig+new kernel, old kernel + new mdconfig, new mdconfig+new kernel) everything seems to be working properly.

sys/sys/mdioctl.h
52 ↗(On Diff #27626)

I think there is alignment in play, the struct is not packed.

rpokala added inline comments.
sbin/mdconfig/mdconfig.8
197 ↗(On Diff #27626)

I believe the rule for manpages is that sentences start on a new line. Also, "can be then" should be "can then be". Finally, the second sentence is a fragment. I suggest this instead:

.It Fl L Ar label
Associate a label (arbitrary string) with the new memory disk.
The label can then be inspected with the
.Nm
.Fl l v
command.
sys/dev/md/md.c
1703 ↗(On Diff #27626)

I think you want copystr().

bcr added a subscriber: bcr.Apr 22 2017, 2:03 PM

Igor suggestion for the man page change.

sbin/mdconfig/mdconfig.8
197 ↗(On Diff #27626)

That's right. Use textproc/igor to check the man page for common errors.

bjk added a subscriber: bjk.Apr 22 2017, 6:54 PM

I've checked both forward and backward compatibility (3-way actually, old mdconfig+new kernel, old kernel + new mdconfig, new mdconfig+new kernel) everything seems to be working properly.

On all supported architectures?

Don't forget to bump .Dd in the man page when committing.

sbin/mdconfig/mdconfig.8
197 ↗(On Diff #27626)

I would prefer "can then be instpected with
.Nm
.Fl l v ."
to eliminate the not-really-needed "the" and "command".

rpokala added inline comments.Apr 22 2017, 11:32 PM
sbin/mdconfig/mdconfig.8
197 ↗(On Diff #27626)

Yeah, that's probably better way to phrase it.

sobomax added inline comments.Apr 23 2017, 1:42 AM
sys/dev/md/md.c
1703 ↗(On Diff #27626)

Are you sure? Reading copystr(8):

DESCRIPTION
     The copy functions are designed to copy contiguous data from one address
     to another.  All but copystr() copy data from user-space to kernel-space
     or vice-versa.
...
     The copystr() function copies a NUL-terminated string, at most len bytes
     long, from kernel-space address kfaddr to kernel-space address kdaddr.
     The number of bytes actually copied, including the terminating NUL, is
     returned in *done (if done is non-NULL).
sobomax updated this revision to Diff 27647.Apr 23 2017, 1:45 AM

Improve wording as suggested by the rpokala, bcr & bjk.

sobomax marked 4 inline comments as done.Apr 23 2017, 1:46 AM
sobomax marked an inline comment as done.Apr 23 2017, 1:57 AM
sobomax added inline comments.
sys/sys/mdioctl.h
52 ↗(On Diff #27626)

I've verified again, the sizeof(struct md_ioctl) remains 448 bytes before and after change on x64. I don't have access to any other supported 64-bit arches to verify.

rpokala added inline comments.Apr 23 2017, 3:13 AM
sys/dev/md/md.c
1703 ↗(On Diff #27626)

You're right. I need new glasses. :-p

wblock added a subscriber: wblock.May 5 2017, 3:10 PM
wblock added inline comments.
sbin/mdconfig/mdconfig.8
197 ↗(On Diff #27626)

I feel like this example command should be separated from the rest of the sentence. Maybe:

The label can then be inspected with
.Bd -literal -offset indent
.Nm
.Fl l v
.Ed

(Might need a .Pp after that.)

sobomax updated this revision to Diff 28650.May 22 2017, 4:39 AM

Reformat in-line example based on input from wblock.

sobomax marked an inline comment as done.May 22 2017, 4:41 AM
sobomax added inline comments.
sbin/mdconfig/mdconfig.8
197 ↗(On Diff #27626)

Done in the latest revision. I have had to merge Nm + Fl... into a single line, though otherwise it would wrap it up separating -l -v to the next line.

wblock accepted this revision.Jul 14 2017, 3:38 PM

Man page changes LGTM.

sbin/mdconfig/mdconfig.8
40 ↗(On Diff #28650)

Remember to bump this on commit.

This revision is now accepted and ready to land.Jul 14 2017, 3:39 PM
This revision was automatically updated to reflect the committed changes.
sobomax marked an inline comment as done.