| Message ID | 20221213172935.680971-8-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs-verity support for XFS | expand |
On Tue, Dec 13, 2022 at 06:29:31PM +0100, Andrey Albershteyn wrote: > The direct path is not supported on verity files. Attempts to use direct > I/O path on such files should fall back to buffered I/O path. > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > --- > fs/xfs/xfs_file.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 5eadd9a37c50e..fb4181e38a19d 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -245,7 +245,8 @@ xfs_file_dax_read( > struct kiocb *iocb, > struct iov_iter *to) > { > - struct xfs_inode *ip = XFS_I(iocb->ki_filp->f_mapping->host); > + struct inode *inode = iocb->ki_filp->f_mapping->host; > + struct xfs_inode *ip = XFS_I(inode); > ssize_t ret = 0; > > trace_xfs_file_dax_read(iocb, to); > @@ -298,10 +299,17 @@ xfs_file_read_iter( > > if (IS_DAX(inode)) > ret = xfs_file_dax_read(iocb, to); fsverity is supported on DAX? Eric, I was under the impression that the DAX io path does not support fsverity, but I can't see anything that prevents ext4 from using fsverity on dax enabled filesystems. Does this work (is it tested regularly?), or is the lack of checking simply an oversight in that nobody thought to check DAX status when fsverity is enabled? > - else if (iocb->ki_flags & IOCB_DIRECT) > + else if (iocb->ki_flags & IOCB_DIRECT && !fsverity_active(inode)) > ret = xfs_file_dio_read(iocb, to); > - else > + else { > + /* > + * In case fs-verity is enabled, we also fallback to the > + * buffered read from the direct read path. Therefore, > + * IOCB_DIRECT is set and need to be cleared > + */ > + iocb->ki_flags &= ~IOCB_DIRECT; > ret = xfs_file_buffered_read(iocb, to); > + } Is this IOCB_DIRECT avoidance a limitation of the XFS implementation, or a generic limitation of the fsverity infrastructure? If it's a limitation of the fsverity infrastructure, then we shouldn't be working around this in every single filesystem that supports fsverity. If all the major filesystems are having to check fsverity_active() and clear IOCB_DIRECT on every single IOCB_DIRECT IO that is issued on a fsverity inode, then shouldn't we just elide IOCB_DIRECT from file->f_iocb_flags in the first place? Cheers, Dave.
On Wed, Dec 14, 2022 at 01:07:15PM +1100, Dave Chinner wrote: > On Tue, Dec 13, 2022 at 06:29:31PM +0100, Andrey Albershteyn wrote: > > The direct path is not supported on verity files. Attempts to use direct > > I/O path on such files should fall back to buffered I/O path. > > > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > > --- > > fs/xfs/xfs_file.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > index 5eadd9a37c50e..fb4181e38a19d 100644 > > --- a/fs/xfs/xfs_file.c > > +++ b/fs/xfs/xfs_file.c > > @@ -245,7 +245,8 @@ xfs_file_dax_read( > > struct kiocb *iocb, > > struct iov_iter *to) > > { > > - struct xfs_inode *ip = XFS_I(iocb->ki_filp->f_mapping->host); > > + struct inode *inode = iocb->ki_filp->f_mapping->host; > > + struct xfs_inode *ip = XFS_I(inode); > > ssize_t ret = 0; > > > > trace_xfs_file_dax_read(iocb, to); > > @@ -298,10 +299,17 @@ xfs_file_read_iter( > > > > if (IS_DAX(inode)) > > ret = xfs_file_dax_read(iocb, to); > > fsverity is supported on DAX? > > Eric, I was under the impression that the DAX io path does not > support fsverity, but I can't see anything that prevents ext4 from > using fsverity on dax enabled filesystems. Does this work (is it > tested regularly?), or is the lack of checking simply an oversight > in that nobody thought to check DAX status when fsverity is enabled? DAX and fsverity are mutually exclusive. ext4_set_inode_flags() doesn't set the DAX flag if the inode already has the verity flag, and ext4_begin_enable_verity() doesn't allow setting the verity flag if the inode already has the DAX flag. > > > - else if (iocb->ki_flags & IOCB_DIRECT) > > + else if (iocb->ki_flags & IOCB_DIRECT && !fsverity_active(inode)) > > ret = xfs_file_dio_read(iocb, to); > > - else > > + else { > > + /* > > + * In case fs-verity is enabled, we also fallback to the > > + * buffered read from the direct read path. Therefore, > > + * IOCB_DIRECT is set and need to be cleared > > + */ > > + iocb->ki_flags &= ~IOCB_DIRECT; > > ret = xfs_file_buffered_read(iocb, to); > > + } > > Is this IOCB_DIRECT avoidance a limitation of the XFS > implementation, or a generic limitation of the fsverity > infrastructure? > > If it's a limitation of the fsverity infrastructure, then we > shouldn't be working around this in every single filesystem that > supports fsverity. If all the major filesystems are having to check > fsverity_active() and clear IOCB_DIRECT on every single IOCB_DIRECT > IO that is issued on a fsverity inode, then shouldn't we just elide > IOCB_DIRECT from file->f_iocb_flags in the first place? It's mainly a filesystem limitation, not a fs/verity/ limitation. However, the functions in fs/verity/verify.c do assume that the data pages are page cache pages. To allow filesystems to support direct I/O on verity files, functions that take the inode and file offset explicitly would need to be added. Not setting IOCB_DIRECT in ->f_iocb_flags is an interesting idea. I've been trying not to add fscrypt and fsverity stuff to the core VFS syscall paths, since only certain filesystems support these features, so it makes sense to limit to the overhead (however minimal) to those filesystems only. However, since ->f_iocb_flags was recently added to cache iocb_flags(), it does look like the VFS could check IS_VERITY() in iocb_flags() with minimal overhead. A potential issue is that if a file is opened with O_DIRECT and then FS_IOC_ENABLE_VERITY is run (either on that fd or on a different fd), then the O_DIRECT fd would still exist -- with IOCB_DIRECT in ->f_iocb_flags. The read-time check would be needed to correctly handle that case. - Eric
On Tue, Dec 13, 2022 at 06:29:31PM +0100, Andrey Albershteyn wrote: > The direct path is not supported on verity files. Attempts to use direct > I/O path on such files should fall back to buffered I/O path. Just curious: why? What prevents us from running the hash function over the user pages in direct I/O?
On Fri, Dec 23, 2022 at 08:18:07AM -0800, Christoph Hellwig wrote: > On Tue, Dec 13, 2022 at 06:29:31PM +0100, Andrey Albershteyn wrote: > > The direct path is not supported on verity files. Attempts to use direct > > I/O path on such files should fall back to buffered I/O path. > > Just curious: why? What prevents us from running the hash function > over the user pages in direct I/O? > hmm, not sure if there are any technical obstacles (probably no), but as fs-verity subsystem doesn't deal with direct i/o, I went the same path.
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 5eadd9a37c50e..fb4181e38a19d 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -245,7 +245,8 @@ xfs_file_dax_read( struct kiocb *iocb, struct iov_iter *to) { - struct xfs_inode *ip = XFS_I(iocb->ki_filp->f_mapping->host); + struct inode *inode = iocb->ki_filp->f_mapping->host; + struct xfs_inode *ip = XFS_I(inode); ssize_t ret = 0; trace_xfs_file_dax_read(iocb, to); @@ -298,10 +299,17 @@ xfs_file_read_iter( if (IS_DAX(inode)) ret = xfs_file_dax_read(iocb, to); - else if (iocb->ki_flags & IOCB_DIRECT) + else if (iocb->ki_flags & IOCB_DIRECT && !fsverity_active(inode)) ret = xfs_file_dio_read(iocb, to); - else + else { + /* + * In case fs-verity is enabled, we also fallback to the + * buffered read from the direct read path. Therefore, + * IOCB_DIRECT is set and need to be cleared + */ + iocb->ki_flags &= ~IOCB_DIRECT; ret = xfs_file_buffered_read(iocb, to); + } if (ret > 0) XFS_STATS_ADD(mp, xs_read_bytes, ret);
The direct path is not supported on verity files. Attempts to use direct I/O path on such files should fall back to buffered I/O path. Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> --- fs/xfs/xfs_file.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)