Selectively refreshing disks

[dlowe]

When running Disks::new_with_refreshed_list() on android devices, I often see warnings (from SELinux?) along the lines of:

type=1400 audit(0.0:339351): avc:  denied  { search } for  name="block" dev="tmpfs" ino=12

I've done some spelunking, and it appears they are a side-effect of this canonicalize call, on certain disks' mount points.

(N.b. the dev="tmpfs" in the warning doesn't refer to the type of the filesystem being refreshed. I'm not using linux-tmpfs, so they're filtered out. In debugging, I've seen the warning for "ext4" and "fuse" filesystems.)

I know these warnings are harmless! ...but I've had customers who were alarmed about them, so I'd prefer to avoid generating them. Given my use-case (I'm only reporting disk usage over time for one disk), I'd love some sort of mechanism for selectively refreshing Disks. There are lots of options.

My first suggestion would be a separate Disks constructor (new_with_list() or new_with_unrefreshed_list()?) that loads the list of disks from /proc/mounts, but defers the stat collection until a later refresh. On linux, at least, I think the separation makes sense, and it gives complete control over what is refreshed to the library user, without adding much complexity to the interface. On other platforms, I haven't looked, but it seems like the worst-case would simply be that new_with_list() calls new_with_refreshed_list() on platforms where there's no distinction between listing and refreshing.

I'd be happy to do the work. If my proposal sounds reasonable, I'll submit a PR next week and we can hash out the specifics. If not, I'd be open to other ways of solving this... just let me know :)

[GuillaumeGomez]

Seems a bit weird to want to have disks but not their path or even their kind. I think the best course would be to instead detect when the canonicalize call is actually needed and only call it if so.

[dlowe]

Seems a bit weird to want to have disks but not their path or even their kind.

I was thinking those would be present in the new_with_list return. Only the information obtained from statvfs would be deferred.

I think the best course would be to instead detect when the canonicalize call is actually needed and only call it if so.

Hummm. Canonicalize (on linux) IIUC is just realpath. How would one decide if it were necessary? (I think you'd end up reimplementing almost all of realpath, right?)

[GuillaumeGomez]

The kind is retrieved from the function using canonicalize, so both functions would get a call to it.

Hummm. Canonicalize (on linux) IIUC is just realpath. How would one decide if it were necessary? (I think you'd end up reimplementing almost all of realpath, right?)

Mostly checking if it's a link, if not, checking if it doesn't start with / or contains .. (still not great to check that ourselves).

Any clue why statvfs is emitting this warning?

[dlowe]

The kind is retrieved from the function using canonicalize, so both functions would get a call to it.

Ah, I see. I was thinking about vfstype, not DiskKind. You're right that DiskKind requires the canonicalize.

So in my suggestion, I guess the populated fields would be device_name, file_system and mount_point; the other fields would require a refresh.

Any clue why statvfs is emitting this warning?

I believe it happens during the canonicalize, not the statvfs. And it's coming from SELinux or apparmor or some similar system for permissions above and beyond POSIX. The fact that it emits these audit warnings in addition to failing the syscall is super annoying, at least in this instance :/

[GuillaumeGomez]

I think DiskKind is part of the fundamental information for a Disk, so I'm not super open to the idea of not having it by default.

However the RefreshKind approach for Disks sounds pretty good, just not sure there are enough different fields to make it relevant currently.

[dlowe]

I'm not sure what you mean about RefreshKind, can you elaborate?

[GuillaumeGomez]

Sure. Like for refresh_processes_specifics: you can pick what you want to refresh with ProcessRefreshKind.

[dlowe]

Ahh, I see. That's perfect! I'd be happy to extend that pattern over Disks. So I think I would:

• add a DiskRefreshKind
• add a Disks::new_with_specifics() constructor
• add a Disks::refresh_specifics() refresher
• add a Disk::refresh_specifics() refresher
• make (at least) the linux impl respect the specific refresh kind(s)

... sound about right? That would work for me :)

[GuillaumeGomez]

Yes but that wouldn't solve your problem since I still want DiskKind to be retrieved for all Disk unconditionally. ^^'

[dlowe]

What if you always get DiskKind unless you specifically ask not to get it?

[GuillaumeGomez]

That could work. Only question, what value do you put into DiskKind::Unknown? It's supposed to mean something to the OS.

[dlowe]

I'm not sure. I don't use the DiskKind, personally. The idiomatic Rust thing would be to make it Option<DiskKind>, but I'm guessing that's exactly what you want to avoid. Could we add another enum variant (DiskKind::NeedsReload)?

[GuillaumeGomez]

Indeed, I don't want an Option. I'm still wondering if we're not trying to go around a technical issue that could be fixed instead...

[dlowe]

FWIW: even if it weren't for the annoying warnings, I would still use this functionality, since I'm currently loading way more information than I actually need :)

[GuillaumeGomez]

We can do it in two passes then: a PR to implement the selective refresh and another where we can debate what's best for DiskKind.

[dlowe]

Look for a PR early next week!

[GuillaumeGomez]

Thanks in advance!