On Wed, Sep 15, 2021 at 02:12:37PM -0700, Eric Biggers wrote: > On Wed, Sep 15, 2021 at 02:01:12PM -0700, Boris Burkov wrote: > > On Wed, Sep 15, 2021 at 01:45:23PM -0700, Eric Biggers wrote: > > > On Tue, Sep 14, 2021 at 11:34:29AM -0700, Boris Burkov wrote: > > > > > Okay, so it is used. (Due to the macro, it didn't show up when grepping.) > > > > > > > > > > Doesn't it defeat the purpose of a ro_compat inode flag if the whole filesystem > > > > > is marked with a ro_compat feature flag, though? I thought that the point of > > > > > the ro_compat inode flag is to allow old kernels to mount the filesystem > > > > > read-write, with only verity files being forced to read-only. That would be > > > > > more flexible than ext4's implementation of fs-verity which forces the whole > > > > > filesystem to read-only. But it seems you're forcing the whole filesystem to > > > > > read-only anyway? > > > > > > > > > > - Eric > > > > > > > > I was thinking of it in terms of "RO compat is the goal" and having new > > > > inode flags totally broke that and was treated as a corruption of the > > > > inode regardless of the fs being ro/rw. I think a check on a live fs > > > > would just flip the fs ro, which was the goal anyway, but a check that > > > > happened during mount would fail the mount, even for a read-only fs. > > > > > > > > Making it fully per file would be pretty cool! The only thing > > > > really missing as far as I can tell is a way to mark a file read only > > > > with the same semantics fsverity uses from within btrfs. > > > > > > I don't understand. Why are you bothering with the ro_compat inode flag at all > > > if it doesn't actually work? > > > > > > - Eric > > > > Sorry I explained that really badly. > > > > My first try was ro-compat bit only, that failed because btrfs couldn't > > add an inode flag in a ro-compat way before my changes, as it could > > fail to mount. > > > > To fix that, I had to work on the inode flag compatibility, which > > evolved into this notion of inode ro-compat flags, which does work as > > expected: if you see a file with an unknown ro-compat flag it's an error > > if you aren't read-only. Read-only mount will never fail. > > > > I think changing the semantics of the ro-compat inodes from: > > "an unknown ro inode flag -> fs ro" to > > "an unknown ro inode flag -> file ro" > > could be a big win. I don't think there is a rush to do that, though? > > If you're forcing the filesystem to read-only anyway, why not just rely on the > filesystem-wide ro_compat flag, which you already implemented and which already > does that? What benefit does the per-file ro_compat flag have, if it doesn't > actually make just the file read-only (which would be the expected behavior)? > You might as well just use a "regular" inode flag in that case. > > - Eric I couldn't use a regular inode flag because the btrfs tree checker will call it an error when it sees a flag it doesn't recognize, regardless of compat bits or fs read-only status. This is extra painful if the inode with verity enabled is in a leaf that gets read in at mount time and gets checked then. a fake example of what was happening: mkfs.btrfs dev mount dev mnt touch /mnt/foo fsverity enable /mnt/foo <reboot to old kernel> mount dev mnt !!!FAIL!!! mount -o ro dev mnt !!!FAIL!!! To get around this, I added a new flag field that wasn't checked as aggressively -- and didn't call it an error on ro mount. There is more excruciating detail, that I won't poorly re-create here, in the commit message of: "btrfs: add ro compat flags to inodes" However, I really do agree that having done the work to add the new class of flags, it makes sense to try to take advantage of it the way you suggest, since per-file ro compat sounds a lot cooler than fs ro compat. I was just trying to do what I could to make the fs compat bit work at all.
|