On Tue, Jan 21, 2020 at 03:07:11PM +0100, Jan Synacek wrote:
> The following patch attempts to implement sparsification of
> LUKS-encrypted partitions. It uses lsblk to pair the underlying LUKS
> block device with its mapped name. Also, --allow-discards was added
> by default to luks_open().
>
> There are several potential issues that I can think of:
>
> 1) If and entire device is encrypted (not just one of more partitions),
> the lsblk trick might not work.
>
> 2) The --allow-discards is needed to be able to run fstrim on a
> decrypted partition. I *think* that it's safe to be added
> unconditionally,
My concerns about making --allow-discards unconditional would be:
* If old versions of cryptsetup supported it at all.
The option was added in cryptsetup 1.4 in Oct 2011, so that's not an
issue.
* If it breaks cryptsetup in any situation.
>From a casual look at libdevmapper it seems like some devices don't
support discards. libdevmapper issues a log message and actually
retries in certain situations, but I'm not sure if that applies to
luksOpen.
* If people opening luks partitions would want to disallow discards.
Not sure.
> but I'm not sure. It might be better to just add
> another luks_open() variant that uses the option.
We can add optional flags to existing APIs. This is better than
adding new APIs. Adding a flag is probably the safest choice since it
punts the decision to the caller and it won't break existing API
users.
To add new opt arguments, add them to the second list (currently []
for luks_open). See for example:
https://github.com/libguestfs/libguestfs/blob/a754cd43078e43f1a2b5d10e54b684c70c5525d7/generator/actions_core.ml#L213
Because the existing API does not have optional arguments you must add
‘once_had_no_optargs = true’ so that the generator adds the backwards
compatibility API.
> 3) As it is right now, lsblk is called for every crypto_LUKS device to
> see if a corresponding mapping had been created. I *think* it's good
> enough, but keeping a list of (blkdev, mapname) in the daemon memory
> and adding an API call to retrieve it might be better.
I'm fairly sure this _isn't_ a good plan since other APIs would update
and invalidate this cache. Do the simple thing. If it's slow then we
can fix that later.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
