From: Eric Biggers <ebiggers@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Andrey Albershteyn <aalbersh@redhat.com>,
linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [RFC PATCH 06/11] xfs: initialize fs-verity on file open and cleanup on inode destruction
Date: Tue, 13 Dec 2022 21:25:38 -0800 [thread overview]
Message-ID: <Y5leUu9cnFbN0OM1@sol.localdomain> (raw)
In-Reply-To: <20221214013524.GF3600936@dread.disaster.area>
On Wed, Dec 14, 2022 at 12:35:24PM +1100, Dave Chinner wrote:
> On Tue, Dec 13, 2022 at 06:29:30PM +0100, Andrey Albershteyn wrote:
> > fs-verity will read and attach metadata (not the tree itself) from
> > a disk for those inodes which already have fs-verity enabled.
> >
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> > fs/xfs/xfs_file.c | 8 ++++++++
> > fs/xfs/xfs_super.c | 2 ++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 242165580e682..5eadd9a37c50e 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -32,6 +32,7 @@
> > #include <linux/mman.h>
> > #include <linux/fadvise.h>
> > #include <linux/mount.h>
> > +#include <linux/fsverity.h>
> >
> > static const struct vm_operations_struct xfs_file_vm_ops;
> >
> > @@ -1170,9 +1171,16 @@ xfs_file_open(
> > struct inode *inode,
> > struct file *file)
> > {
> > + int error = 0;
> > +
> > if (xfs_is_shutdown(XFS_M(inode->i_sb)))
> > return -EIO;
> > file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC;
> > +
> > + error = fsverity_file_open(inode, file);
> > + if (error)
> > + return error;
>
> This is a hot path, so shouldn't we elide the function call
> altogether if verity is not enabled on the inode? i.e:
>
> if (IS_VERITY(inode)) {
> error = fsverity_file_open(inode, file);
> if (error)
> return error;
> }
>
> It doesn't really matter for a single file open, but when you're
> opening a few million inodes every second the function call overhead
> only to immediately return because IS_VERITY() is false adds up...
>
> > return generic_file_open(inode, file);
> > }
> >
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 8f1e9b9ed35d9..50c2c819ba940 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -45,6 +45,7 @@
> > #include <linux/magic.h>
> > #include <linux/fs_context.h>
> > #include <linux/fs_parser.h>
> > +#include <linux/fsverity.h>
> >
> > static const struct super_operations xfs_super_operations;
> >
> > @@ -647,6 +648,7 @@ xfs_fs_destroy_inode(
> > ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> > XFS_STATS_INC(ip->i_mount, vn_rele);
> > XFS_STATS_INC(ip->i_mount, vn_remove);
> > + fsverity_cleanup_inode(inode);
>
> Similarly, shouldn't this be:
>
> if (fsverity_active(inode))
> fsverity_cleanup_inode(inode);
>
If you actually want to do that, then we should instead make these functions
inline functions that do the "is anything needed?" check, then call a
double-underscored version that does the actual work. Some of the fscrypt
functions are like that. Then all filesystems would get the benefit.
Funnily enough, I had actually wanted to do that for fsverity_file_open()
originally, but Ted had preferred the simpler version.
Anyway, if this is something you want, I can change it to be that way.
- Eric
next prev parent reply other threads:[~2022-12-14 5:25 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-13 17:29 [RFC PATCH 00/11] fs-verity support for XFS Andrey Albershteyn
2022-12-13 17:29 ` [RFC PATCH 01/11] xfs: enable large folios in xfs_setup_inode() Andrey Albershteyn
2022-12-14 0:53 ` Dave Chinner
2022-12-13 17:29 ` [RFC PATCH 02/11] pagemap: add mapping_clear_large_folios() wrapper Andrey Albershteyn
2022-12-13 17:55 ` Matthew Wilcox
2022-12-13 19:33 ` Eric Biggers
2022-12-13 21:10 ` Dave Chinner
2022-12-14 6:52 ` Eric Biggers
2022-12-14 8:12 ` Dave Chinner
2022-12-13 21:08 ` Dave Chinner
2023-01-09 16:34 ` Andrey Albershteyn
2022-12-13 17:29 ` [RFC PATCH 03/11] xfs: add attribute type for fs-verity Andrey Albershteyn
2022-12-13 17:43 ` Eric Sandeen
2022-12-14 1:03 ` Dave Chinner
2023-01-09 16:37 ` Andrey Albershteyn
2022-12-13 17:29 ` [RFC PATCH 04/11] xfs: add fs-verity ro-compat flag Andrey Albershteyn
2022-12-14 1:06 ` Dave Chinner
2022-12-13 17:29 ` [RFC PATCH 05/11] xfs: add inode on-disk VERITY flag Andrey Albershteyn
2022-12-14 1:29 ` Dave Chinner
2023-01-09 16:51 ` Andrey Albershteyn
2022-12-13 17:29 ` [RFC PATCH 06/11] xfs: initialize fs-verity on file open and cleanup on inode destruction Andrey Albershteyn
2022-12-14 1:35 ` Dave Chinner
2022-12-14 5:25 ` Eric Biggers [this message]
2022-12-14 8:18 ` Dave Chinner
2022-12-13 17:29 ` [RFC PATCH 07/11] xfs: disable direct read path for fs-verity sealed files Andrey Albershteyn
2022-12-14 2:07 ` Dave Chinner
2022-12-14 5:44 ` Eric Biggers
2022-12-23 16:18 ` Christoph Hellwig
2023-01-09 17:23 ` Andrey Albershteyn
2022-12-13 17:29 ` [RFC PATCH 08/11] xfs: don't enable large folios on fs-verity sealed inode Andrey Albershteyn
2022-12-14 2:07 ` Dave Chinner
2022-12-13 17:29 ` [RFC PATCH 09/11] iomap: fs-verity verification on page read Andrey Albershteyn
2022-12-13 19:02 ` Eric Biggers
2023-01-09 16:58 ` Andrey Albershteyn
2022-12-14 5:43 ` Dave Chinner
2022-12-13 17:29 ` [RFC PATCH 10/11] xfs: add fs-verity support Andrey Albershteyn
2022-12-13 19:08 ` Eric Biggers
2022-12-13 19:22 ` Darrick J. Wong
2022-12-13 20:13 ` Eric Biggers
2022-12-13 20:33 ` Dave Chinner
2022-12-13 20:39 ` Eric Biggers
2022-12-13 21:40 ` Dave Chinner
2022-12-14 7:58 ` Dave Chinner
2022-12-13 17:29 ` [RFC PATCH 11/11] xfs: add fs-verity ioctls Andrey Albershteyn
2022-12-13 20:50 ` [RFC PATCH 00/11] fs-verity support for XFS Eric Biggers
2022-12-13 22:11 ` Dave Chinner
2022-12-14 6:31 ` Eric Biggers
2022-12-14 23:06 ` Dave Chinner
2022-12-15 6:47 ` Eric Biggers
2022-12-15 20:57 ` Dave Chinner
2022-12-16 5:04 ` Eric Biggers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y5leUu9cnFbN0OM1@sol.localdomain \
--to=ebiggers@kernel.org \
--cc=aalbersh@redhat.com \
--cc=david@fromorbit.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).