| Message ID | 20230523081322.331337-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] btrfs: fix range_end calculation in extent_write_locked_range | expand |
On 2023/5/23 16:13, Christoph Hellwig wrote: > Split all the conditionals for the fsverity calls in end_page_read into > a btrfs_verify_page helper to keep the code readable and make additional > refacoring easier. I know this patch is only doing refactoring, but this makes me wonder, should we make the btrfs_verify_page() to be subpage compatible? E.g. checking subpage page error and page uptodate? Thanks, Qu > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/extent_io.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index c1b0ca94be34e1..fc48888742debd 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -481,6 +481,15 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end, > start, end, page_ops, NULL); > } > > +static int btrfs_verify_page(struct page *page, u64 start) > +{ > + if (!fsverity_active(page->mapping->host) || > + PageError(page) || PageUptodate(page) || > + start >= i_size_read(page->mapping->host)) > + return true; > + return fsverity_verify_page(page); > +} > + > static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len) > { > struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb); > @@ -489,11 +498,7 @@ static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len) > start + len <= page_offset(page) + PAGE_SIZE); > > if (uptodate) { > - if (fsverity_active(page->mapping->host) && > - !PageError(page) && > - !PageUptodate(page) && > - start < i_size_read(page->mapping->host) && > - !fsverity_verify_page(page)) { > + if (!btrfs_verify_page(page, start)) { > btrfs_page_set_error(fs_info, page, start, len); > } else { > btrfs_page_set_uptodate(fs_info, page, start, len);
On 23/05/2023 16:13, Christoph Hellwig wrote: > Split all the conditionals for the fsverity calls in end_page_read into > a btrfs_verify_page helper to keep the code readable and make additional > refacoring easier. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/extent_io.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index c1b0ca94be34e1..fc48888742debd 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -481,6 +481,15 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end, > start, end, page_ops, NULL); > } > > +static int btrfs_verify_page(struct page *page, u64 start) Did you consider making it an inline function? Thanks, Anand > +{ > + if (!fsverity_active(page->mapping->host) || > + PageError(page) || PageUptodate(page) || > + start >= i_size_read(page->mapping->host)) > + return true; > + return fsverity_verify_page(page); > +} > + > static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len) > { > struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb); > @@ -489,11 +498,7 @@ static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len) > start + len <= page_offset(page) + PAGE_SIZE); > > if (uptodate) { > - if (fsverity_active(page->mapping->host) && > - !PageError(page) && > - !PageUptodate(page) && > - start < i_size_read(page->mapping->host) && > - !fsverity_verify_page(page)) { > + if (!btrfs_verify_page(page, start)) { > btrfs_page_set_error(fs_info, page, start, len); > } else { > btrfs_page_set_uptodate(fs_info, page, start, len);
On Tue, May 23, 2023 at 05:23:19PM +0800, Qu Wenruo wrote: > I know this patch is only doing refactoring, but this makes me wonder, > should we make the btrfs_verify_page() to be subpage compatible? > > E.g. checking subpage page error and page uptodate? Not without someone signing up to actually fully test and support fsverity on sub-page file systems.
On Tue, May 23, 2023 at 06:16:25PM +0800, Anand Jain wrote: >> +static int btrfs_verify_page(struct page *page, u64 start) > > Did you consider making it an inline function? No, why?
On 23/05/2023 19:39, Christoph Hellwig wrote: > On Tue, May 23, 2023 at 06:16:25PM +0800, Anand Jain wrote: >>> +static int btrfs_verify_page(struct page *page, u64 start) >> >> Did you consider making it an inline function? > > No, why? As the compiler optimization may or may not inline it?
On Tue, May 23, 2023 at 07:47:14PM +0800, Anand Jain wrote: > > > On 23/05/2023 19:39, Christoph Hellwig wrote: >> On Tue, May 23, 2023 at 06:16:25PM +0800, Anand Jain wrote: >>>> +static int btrfs_verify_page(struct page *page, u64 start) >>> >>> Did you consider making it an inline function? >> >> No, why? > > As the compiler optimization may or may not inline it? And making that decision is in general the compilers job. I don't see a need to second guess the compiler here.
On Tue, May 23, 2023 at 10:13:08AM +0200, Christoph Hellwig wrote: > Split all the conditionals for the fsverity calls in end_page_read into > a btrfs_verify_page helper to keep the code readable and make additional > refacoring easier. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/extent_io.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index c1b0ca94be34e1..fc48888742debd 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -481,6 +481,15 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end, > start, end, page_ops, NULL); > } > > +static int btrfs_verify_page(struct page *page, u64 start) This should match return value of fsverity_verify_page() which is bool. > +{ > + if (!fsverity_active(page->mapping->host) || > + PageError(page) || PageUptodate(page) || > + start >= i_size_read(page->mapping->host)) > + return true; This is right, so the 'int' was probably a typo. > + return fsverity_verify_page(page); > +}
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index c1b0ca94be34e1..fc48888742debd 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -481,6 +481,15 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end, start, end, page_ops, NULL); } +static int btrfs_verify_page(struct page *page, u64 start) +{ + if (!fsverity_active(page->mapping->host) || + PageError(page) || PageUptodate(page) || + start >= i_size_read(page->mapping->host)) + return true; + return fsverity_verify_page(page); +} + static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len) { struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb); @@ -489,11 +498,7 @@ static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len) start + len <= page_offset(page) + PAGE_SIZE); if (uptodate) { - if (fsverity_active(page->mapping->host) && - !PageError(page) && - !PageUptodate(page) && - start < i_size_read(page->mapping->host) && - !fsverity_verify_page(page)) { + if (!btrfs_verify_page(page, start)) { btrfs_page_set_error(fs_info, page, start, len); } else { btrfs_page_set_uptodate(fs_info, page, start, len);
Split all the conditionals for the fsverity calls in end_page_read into a btrfs_verify_page helper to keep the code readable and make additional refacoring easier. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/extent_io.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)