On Thu, Jul 22, 2021 at 11:58 PM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 22.07.21 20:00, Evan Green wrote: > > On Thu, Jul 22, 2021 at 12:12 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > >> > >> On 21.07.21 23:40, Evan Green wrote: > >>> Currently it's not possible to enable hibernation without also enabling > >>> generic swap for a given swap area. These two use cases are not the > >>> same. For example there may be users who want to enable hibernation, > >>> but whose drives don't have the write endurance for generic swap > >>> activities. Swap and hibernate also have different security/integrity > >>> requirements, prompting folks to possibly set up something like block-level > >>> integrity for swap and image-level integrity for hibernate. Keeping swap > >>> and hibernate separate in these cases becomes not just a matter of > >>> preference, but correctness. > >>> > >>> Add a new SWAP_FLAG_NOSWAP that adds a swap region but refuses to allow > >>> generic swapping to it. This region can still be wired up for use in > >>> suspend-to-disk activities, but will never have regular pages swapped to > >>> it. This flag will be passed in by utilities like swapon(8), usage would > >>> probably look something like: swapon -o noswap /dev/sda2. > >> > >> Just a minor comment, I'd call it rather SWAP_FLAG_HIBERNATE_ONLY and > >> SWAP_FLAG_HIBERNATE_ONLY -- that calls the child by its name. > > > > I went back and forth on this too. It seemed pretty close to toss-up > > to me. I went with NOSWAP ultimately because it seemed more closely > > tied to what the flag was actually doing, rather than building in my > > one expected use case into the name. In some world years from now > > where either hibernate has diverged, been deleted, or maybe some new > > usage has been invented for swap space, the NOSWAP name felt like it > > had a better chance of holding up. The argument is weak though, as > > these features are pretty well cast in stone, and the likelihood of > > any of those outcomes seems low. I can change it if you feel strongly, > > but would probably keep it as-is otherwise. > > Just imagine technology Z popping up and using also the swap > infrastructure. What would be the semantics of NOSWAP? With > HIBERNATE_ONLY it's clear -- enable that device only for hibernation, > nothing else. > > But you raise a good point: if hibernation isn't even possible in a > configuration (e.g., not configured into the kernel), we should simply > reject that flag. So if hibernation would vanish at some point > completely from the system, it would all be handled accordingly. > > That would result in quite a consistent definition of > SWAP_FLAG_HIBERNATE_ONLY IMHO. > > Makes sense? Ok, I'll change the name, and reject the flag if hibernation is not enabled. > > > > >> > >> I think some other flags might not apply with that new flag set, right? > >> For example, does SWAP_FLAG_DISCARD_ONCE or SWP_AREA_DISCARD still have > >> any meaning with the new flag being set? > >> > >> We should most probably disallow enabling any flag that doesn't make any > >> sense in combination. > > > > Good point, I can send a followup patch for that. From my reading > > I'd actually enjoy if we'd have that logic in the introducing patch. Ok. > > > SWAP_FLAG_DISCARD and SWAP_FLAG_DISCARD_ONCE are still valid, since > > the discard can be run at swapon() time. SWAP_FLAG_PREFER (specifying > > the priority) doesn't make sense, and SWAP_FLAG_DISCARD_PAGES never > > kicks in because it's called at the cluster level. Hm, that sort of > > seems like a bug that freed hibernate swap doesn't get discarded. I > > can disallow it now as unsupported, but might send a patch to fix it > > later. > > Might be worth fixing, indeed. > > > > >> > >> Apart from that, I'd love to see a comment in here why the workaround > >> suggested by Michal isn't feasible -- essentially a summary of what we > >> discussed. > > > > Ah sorry, I had tried to clarify that in the commit text, but didn't > > explicitly address the workaround. To summarize, the workaround keeps > > generic swap out of your hibernate region... until hibernate time. But > > once hibernate starts, a lot of swapping tends to happen when the > > hiber-image is allocated. At this point the hibernate region is > > eligible for general swap even with the workaround. The reasons I gave > > for wanting to exclusively steer swap and hibernate are SSD write > > wearing, different integrity solutions for swap vs hibernate, and our > > own security changes that no-op out the swapon/swapoff syscalls after > > init. > > > > That would be nice to have in the patch description :) Sure, I'll add that as well and send out a v4 shortly. -Evan
