| Message ID | 20230404145319.2057051-20-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fs-verity support for XFS | expand |
On Tue, Apr 04, 2023 at 04:53:15PM +0200, 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 947b5c436172..9e072e82f6c1 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -244,7 +244,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); > @@ -297,10 +298,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); XFS doesn't usually allow directio fallback to the pagecache. Why would fsverity be any different? --D > + } > > if (ret > 0) > XFS_STATS_ADD(mp, xs_read_bytes, ret); > -- > 2.38.4 >
On Tue, Apr 04, 2023 at 09:10:47AM -0700, Darrick J. Wong wrote: > On Tue, Apr 04, 2023 at 04:53:15PM +0200, 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 947b5c436172..9e072e82f6c1 100644 > > --- a/fs/xfs/xfs_file.c > > +++ b/fs/xfs/xfs_file.c > > @@ -244,7 +244,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); > > @@ -297,10 +298,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); > > XFS doesn't usually allow directio fallback to the pagecache. Why > would fsverity be any different? Didn't know that, this is what happens on ext4 so I did the same. Then it probably make sense to just error on DIRECT on verity sealed file. > > --D > > > + } > > > > if (ret > 0) > > XFS_STATS_ADD(mp, xs_read_bytes, ret); > > -- > > 2.38.4 > > >
On Wed, Apr 05, 2023 at 05:01:42PM +0200, Andrey Albershteyn wrote: > On Tue, Apr 04, 2023 at 09:10:47AM -0700, Darrick J. Wong wrote: > > On Tue, Apr 04, 2023 at 04:53:15PM +0200, 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 947b5c436172..9e072e82f6c1 100644 > > > --- a/fs/xfs/xfs_file.c > > > +++ b/fs/xfs/xfs_file.c > > > @@ -244,7 +244,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); > > > @@ -297,10 +298,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); > > > > XFS doesn't usually allow directio fallback to the pagecache. Why > > would fsverity be any different? > > Didn't know that, this is what happens on ext4 so I did the same. > Then it probably make sense to just error on DIRECT on verity > sealed file. Thinking about this a little more -- I suppose we shouldn't just go breaking directio reads from a verity file if we can help it. Is there a way to ask fsverity to perform its validation against some arbitrary memory buffer that happens to be fs-block aligned? In which case we could support fsblock-aligned directio reads without falling back to the page cache? --D > > > > --D > > > > > + } > > > > > > if (ret > 0) > > > XFS_STATS_ADD(mp, xs_read_bytes, ret); > > > -- > > > 2.38.4 > > > > > > > -- > - Andrey >
On Wed, Apr 05, 2023 at 08:09:27AM -0700, Darrick J. Wong wrote: > Thinking about this a little more -- I suppose we shouldn't just go > breaking directio reads from a verity file if we can help it. Is there > a way to ask fsverity to perform its validation against some arbitrary > memory buffer that happens to be fs-block aligned? That would be my preference as well. But maybe Eric know a good reason why this hasn't been done yet.
On Wed, Apr 05, 2023 at 08:50:10AM -0700, Christoph Hellwig wrote: > On Wed, Apr 05, 2023 at 08:09:27AM -0700, Darrick J. Wong wrote: > > Thinking about this a little more -- I suppose we shouldn't just go > > breaking directio reads from a verity file if we can help it. Is there > > a way to ask fsverity to perform its validation against some arbitrary > > memory buffer that happens to be fs-block aligned? You could certainly add such a function that wraps around verify_data_block(). The minimal function prototype needed (without supporting readahead or reusing the ahash_request) would be something like the following, I think: bool fsverity_verify_blocks_dio(struct inode *inode, u64 pos, struct folio *folio, size_t len, size_t offset); And I really hope that you don't want to do DIO to the *Merkle tree*, as that would make the problem significantly harder. I think DIO for the data, but handling the Merkle tree in the usual way, would be okay? > > That would be my preference as well. But maybe Eric know a good reason > why this hasn't been done yet. > I believe it would be possible, especially if DIO to the Merkle tree is not in scope. There just hasn't been a reason to the work yet. And ext4 and f2fs already fall back to buffer I/O for other filesystem features, so there was precedent for not bothering with DIO, at least in the initial version. - Eric
On Wed, Apr 05, 2023 at 08:09:27AM -0700, Darrick J. Wong wrote: > On Wed, Apr 05, 2023 at 05:01:42PM +0200, Andrey Albershteyn wrote: > > On Tue, Apr 04, 2023 at 09:10:47AM -0700, Darrick J. Wong wrote: > > > On Tue, Apr 04, 2023 at 04:53:15PM +0200, 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 947b5c436172..9e072e82f6c1 100644 > > > > --- a/fs/xfs/xfs_file.c > > > > +++ b/fs/xfs/xfs_file.c > > > > @@ -244,7 +244,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); > > > > @@ -297,10 +298,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); > > > > > > XFS doesn't usually allow directio fallback to the pagecache. Why > > > would fsverity be any different? > > > > Didn't know that, this is what happens on ext4 so I did the same. > > Then it probably make sense to just error on DIRECT on verity > > sealed file. > > Thinking about this a little more -- I suppose we shouldn't just go > breaking directio reads from a verity file if we can help it. Is there > a way to ask fsverity to perform its validation against some arbitrary > memory buffer that happens to be fs-block aligned? The memory buffer doesn't even need to be fs-block aligned - it just needs to be a pointer to memory the kernel can read... We also need fsverity to be able to handle being passed mapped kernel memory rather than pages/folios for the merkle tree interfaces. That way we can just pass it the mapped buffer memory straight from the xfs-buf and we don't have to do the whacky "copy from xattr xfs_bufs into pages so fsverity can take temporary reference counts on what it thinks are page cache pages" as it walks the merkle tree. -Dave.
On Wed, Apr 05, 2023 at 06:02:47PM +0000, Eric Biggers wrote:
> And I really hope that you don't want to do DIO to the *Merkle tree*, as that
Not for XFS - the merkle tree is not held as file data.
That said, the merkle tree in XFS is not page cache or block aligned
metadata either, so we really want the same memory buffer based
interface for the merkle tree reading so that the merkle tree code
can read directly from the xfs-buf rather than requiring us to copy
it out of the xfsbuf into temporary pages...
Cheers,
Dave.
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 947b5c436172..9e072e82f6c1 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -244,7 +244,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); @@ -297,10 +298,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(-)