Page MenuHomeFreeBSD

Add ability to label md(4) devices
ClosedPublic

Authored by sobomax on Apr 22 2017, 3:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 18, 2:54 AM
Unknown Object (File)
Wed, Dec 18, 2:52 AM
Unknown Object (File)
Thu, Nov 28, 7:53 PM
Unknown Object (File)
Nov 21 2024, 5:29 PM
Unknown Object (File)
Nov 21 2024, 6:29 AM
Unknown Object (File)
Nov 19 2024, 4:53 PM
Unknown Object (File)
Nov 15 2024, 6:16 AM
Unknown Object (File)
Nov 10 2024, 5:13 PM

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 9428

Event Timeline

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

sys/sys/mdioctl.h
52

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

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

rpokala added inline comments.
sbin/mdconfig/mdconfig.8
197

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

I think you want copystr().

Igor suggestion for the man page change.

sbin/mdconfig/mdconfig.8
197

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

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

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

sbin/mdconfig/mdconfig.8
197

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

sys/dev/md/md.c
1703

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

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

sobomax added inline comments.
sys/sys/mdioctl.h
52

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.

sys/dev/md/md.c
1703

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

wblock added inline comments.
sbin/mdconfig/mdconfig.8
197

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

Reformat in-line example based on input from wblock.

sobomax added inline comments.
sbin/mdconfig/mdconfig.8
197

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.

Man page changes LGTM.

sbin/mdconfig/mdconfig.8
40

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.