On Mon, Nov 04, 2019 at 04:15:54PM -0800, Christoph Hellwig wrote:

quoted

I don't think combining these things is a good idea because it would restrict
the use of inline encryption to filesystems that allow IV_INO_LBLK_64 encryption
policies, i.e. filesystems that have stable inode numbers, 32-bit inodes, and
32-bit file logical block numbers.

The on-disk format (i.e. the type of encryption policy chosen) and the
implementation (inline or filesystem-layer crypto) are really two separate
things.  This was one of the changes in v4 => v5 of this patchset; these two
things used to be conflated but now they are separate.  Now you can use inline
encryption with the existing fscrypt policies too.

We could use two separate SB_* flags, like SB_INLINE_CRYPT and
SB_IV_INO_LBLK_64_SUPPORT.

Yes, I think that is a good idea.

quoted

However, the ->has_stable_inodes() and
->get_ino_and_lblk_bits() methods are nice because they separate the filesystem
properties from the question of "is this encryption policy supported".
Declaring the filesystem properties is easier to do because it doesn't require
any fscrypt-specific knowledge.  Also, fs/crypto/ could use these properties in
different ways in the future, e.g. if another IV generation scheme is added.

I don't really like writing up method boilerplates for something that
is a simple boolean flag.

fs/crypto/ uses ->has_stable_inodes() and ->get_ino_and_lblk_bits() to print an
appropriate error message.  If we changed it to a simple flag we'd have to print
a less useful error message.  Also, people are basically guaranteed to not
understand what "SB_IV_INO_LBLK_64_SUPPORT" means exactly, and are likely to
copy-and-paste it incorrectly when adding fscrypt support to a new filesystem.
Also it would make it more difficult to add other fscrypt IV generation schemes
in the future as we'd then need to add another sb flag (e.g. SB_IV_INO_LBLK_128)
and make filesystem-specific changes, rather than change fs/crypto/ only.

So personally I'd prefer to keep ->has_stable_inodes() and
->get_ino_and_lblk_bits() for now.

Replacing ->inline_crypt_enabled() with SB_INLINE_CRYPT makes much more sense
though.

- Eric