| Message ID | 20230309090526.332550-18-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/20] btrfs: mark extent_buffer_under_io static | expand |
On 09.03.23 10:08, Christoph Hellwig wrote: > The only place that reads in pages and thus marks them uptodate for > the btree inode is read_extent_buffer_pages. Which means that either > pages are already uptodate from an old buffer when creating a new > one in alloc_extent_buffer, or they will be updated by ca call > to read_extent_buffer_pages. This means the checks for uptodate > pages in read_extent_buffer_pages and read_extent_buffer_subpage are > superflous and can be removed. superfluous ^ [...] Otherwise, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 2023/3/9 17:05, Christoph Hellwig wrote: > The only place that reads in pages and thus marks them uptodate for > the btree inode is read_extent_buffer_pages. Which means that either > pages are already uptodate from an old buffer when creating a new > one in alloc_extent_buffer, or they will be updated by ca call > to read_extent_buffer_pages. This means the checks for uptodate > pages in read_extent_buffer_pages and read_extent_buffer_subpage are > superflous and can be removed. Can we rely completely on EXTENT_BUFFER_UPTODATE flag and getting rid of PateUptodate for metadata? Or the PageUptodate would be shared by some other common routines like readahead? Thanks, Qu > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/extent_io.c | 21 +-------------------- > 1 file changed, 1 insertion(+), 20 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 88a941c64fc73f..73ea618282d466 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -4138,10 +4138,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait, > return ret; > } > > - if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) || > - PageUptodate(page) || > - btrfs_subpage_test_uptodate(fs_info, page, eb->start, eb->len)) { > - set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags); > + if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags)) { > unlock_extent(io_tree, eb->start, eb->start + eb->len - 1, > &cached_state); > return 0; > @@ -4168,7 +4165,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num, > int i; > struct page *page; > int locked_pages = 0; > - int all_uptodate = 1; > int num_pages; > > if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags)) > @@ -4203,21 +4199,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num, > } > locked_pages++; > } > - /* > - * We need to firstly lock all pages to make sure that > - * the uptodate bit of our pages won't be affected by > - * clear_extent_buffer_uptodate(). > - */ > - for (i = 0; i < num_pages; i++) { > - page = eb->pages[i]; > - if (!PageUptodate(page)) > - all_uptodate = 0; > - } > - > - if (all_uptodate) { > - set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags); > - goto unlock_exit; > - } > > __read_extent_buffer_pages(eb, mirror_num, check); >
On Fri, Mar 10, 2023 at 05:08:17PM +0800, Qu Wenruo wrote: >> The only place that reads in pages and thus marks them uptodate for >> the btree inode is read_extent_buffer_pages. Which means that either >> pages are already uptodate from an old buffer when creating a new >> one in alloc_extent_buffer, or they will be updated by ca call >> to read_extent_buffer_pages. This means the checks for uptodate >> pages in read_extent_buffer_pages and read_extent_buffer_subpage are >> superflous and can be removed. > > Can we rely completely on EXTENT_BUFFER_UPTODATE flag and getting rid of > PateUptodate for metadata? > > Or the PageUptodate would be shared by some other common routines like > readahead? That's a good question. Not maintaining PageUptodate would simplify a lot of things. As btree_aops has no ->readpage or ->readahead, the filemap read code is never used, which the biggest dependency on PageUptodate in the core pagecache code. So right now I can't think of anything requiring it, but I'd have to carefully look into that.
On Fri, Mar 10, 2023 at 05:08:17PM +0800, Qu Wenruo wrote: > Can we rely completely on EXTENT_BUFFER_UPTODATE flag and getting rid of > PateUptodate for metadata? I looked into this a bit, and think without stopping to use the page cache entirely (which I want to get to eventually) this causes more trouble than it solves. So I've skipped it for this series, but eventually there will be another round of massive simplifications here.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 88a941c64fc73f..73ea618282d466 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -4138,10 +4138,7 @@ static int read_extent_buffer_subpage(struct extent_buffer *eb, int wait, return ret; } - if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) || - PageUptodate(page) || - btrfs_subpage_test_uptodate(fs_info, page, eb->start, eb->len)) { - set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags); + if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags)) { unlock_extent(io_tree, eb->start, eb->start + eb->len - 1, &cached_state); return 0; @@ -4168,7 +4165,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num, int i; struct page *page; int locked_pages = 0; - int all_uptodate = 1; int num_pages; if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags)) @@ -4203,21 +4199,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num, } locked_pages++; } - /* - * We need to firstly lock all pages to make sure that - * the uptodate bit of our pages won't be affected by - * clear_extent_buffer_uptodate(). - */ - for (i = 0; i < num_pages; i++) { - page = eb->pages[i]; - if (!PageUptodate(page)) - all_uptodate = 0; - } - - if (all_uptodate) { - set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags); - goto unlock_exit; - } __read_extent_buffer_pages(eb, mirror_num, check);
The only place that reads in pages and thus marks them uptodate for the btree inode is read_extent_buffer_pages. Which means that either pages are already uptodate from an old buffer when creating a new one in alloc_extent_buffer, or they will be updated by ca call to read_extent_buffer_pages. This means the checks for uptodate pages in read_extent_buffer_pages and read_extent_buffer_subpage are superflous and can be removed. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/extent_io.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-)