| Message ID | 20240304191046.157464-22-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fs-verity support for XFS | expand |
On Mon, Mar 04, 2024 at 08:10:43PM +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 | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 17404c2e7e31..af3201075066 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -281,7 +281,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); > @@ -334,10 +335,18 @@ 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 { I think the earlier cases need curly braces {} too. > + /* > + * 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 (see > + * generic_file_read_iter()) > + */ > + iocb->ki_flags &= ~IOCB_DIRECT; I'm curious that you added this flag here; how have we gotten along this far without clearing it? --D > ret = xfs_file_buffered_read(iocb, to); > + } > > if (ret > 0) > XFS_STATS_ADD(mp, xs_read_bytes, ret); > -- > 2.42.0 > >
On 2024-03-07 14:11:08, Darrick J. Wong wrote: > On Mon, Mar 04, 2024 at 08:10:43PM +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 | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > index 17404c2e7e31..af3201075066 100644 > > --- a/fs/xfs/xfs_file.c > > +++ b/fs/xfs/xfs_file.c > > @@ -281,7 +281,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); > > @@ -334,10 +335,18 @@ 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 { > > I think the earlier cases need curly braces {} too. > > > + /* > > + * 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 (see > > + * generic_file_read_iter()) > > + */ > > + iocb->ki_flags &= ~IOCB_DIRECT; > > I'm curious that you added this flag here; how have we gotten along > this far without clearing it? > > --D Do you know any better place? Not sure if that should be somewhere before. I've made it same as ext4 does it.
On Tue, Mar 12, 2024 at 01:02:52PM +0100, Andrey Albershteyn wrote: > On 2024-03-07 14:11:08, Darrick J. Wong wrote: > > On Mon, Mar 04, 2024 at 08:10:43PM +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 | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > index 17404c2e7e31..af3201075066 100644 > > > --- a/fs/xfs/xfs_file.c > > > +++ b/fs/xfs/xfs_file.c > > > @@ -281,7 +281,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); > > > @@ -334,10 +335,18 @@ 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 { > > > > I think the earlier cases need curly braces {} too. > > > > > + /* > > > + * 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 (see > > > + * generic_file_read_iter()) > > > + */ > > > + iocb->ki_flags &= ~IOCB_DIRECT; > > > > I'm curious that you added this flag here; how have we gotten along > > this far without clearing it? > > > > --D > > Do you know any better place? Not sure if that should be somewhere > before. I've made it same as ext4 does it. It's not the placement that I'm wondering about, it's /only/ the clearing of IOCB_DIRECT. Notice how directio writes (xfs_file_write_iter) fall back to buffered without clearing that flag: if (iocb->ki_flags & IOCB_DIRECT) { /* * Allow a directio write to fall back to a buffered * write *only* in the case that we're doing a reflink * CoW. In all other directio scenarios we do not * allow an operation to fall back to buffered mode. */ ret = xfs_file_dio_write(iocb, from); if (ret != -ENOTBLK) return ret; } return xfs_file_buffered_write(iocb, from); Or is the problem here that generic_file_read_iter will trip over it? Oh, haha, yes it will. Maybe we should call filemap_read directly then? But I guess this (clearing IOCB_DIRECT) is fine the way it is. --D > -- > - Andrey > >
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 17404c2e7e31..af3201075066 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -281,7 +281,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); @@ -334,10 +335,18 @@ 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 (see + * generic_file_read_iter()) + */ + 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 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)