On Mon, Jun 01, 2015 at 08:52:36PM +0530, Chandan Rajendra wrote:
> For the subpagesize-blocksize scenario, a page can contain multiple
> blocks. In such cases, this patch handles reading data from files.
>
> To track the status of individual blocks of a page, this patch makes use of a
> bitmap pointed to by page->private.
Start going through the patchset, it's not easy though.
Several comments are following.
>
> Signed-off-by: Chandan Rajendra <chandan@xxxxxxxxxxxxxxxxxx>
> ---
> fs/btrfs/extent_io.c | 301 +++++++++++++++++++++++++++++++++------------------
> fs/btrfs/extent_io.h | 28 ++++-
> fs/btrfs/inode.c | 13 +--
> 3 files changed, 224 insertions(+), 118 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 782f3bc..d37badb 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1325,6 +1325,88 @@ int clear_extent_uptodate(struct extent_io_tree *tree, u64 start, u64 end,
> cached_state, mask);
> }
>
> +static int modify_page_blks_state(struct page *page,
> + unsigned long blk_states,
> + u64 start, u64 end, int set)
> +{
> + struct inode *inode = page->mapping->host;
> + unsigned long *bitmap;
> + unsigned long state;
> + u64 nr_blks;
> + u64 blk;
> +
> + BUG_ON(!PagePrivate(page));
> +
> + bitmap = ((struct btrfs_page_private *)page->private)->bstate;
> +
> + blk = (start & (PAGE_CACHE_SIZE - 1)) >> inode->i_blkbits;
> + nr_blks = (end - start + 1) >> inode->i_blkbits;
> +
> + while (nr_blks--) {
> + state = find_next_bit(&blk_states, BLK_NR_STATE, 0);
Looks like we don't need to do find_next_bit for every block.
> +
> + while (state < BLK_NR_STATE) {
> + if (set)
> + set_bit((blk * BLK_NR_STATE) + state, bitmap);
> + else
> + clear_bit((blk * BLK_NR_STATE) + state, bitmap);
> +
> + state = find_next_bit(&blk_states, BLK_NR_STATE,
> + state + 1);
> + }
> +
> + ++blk;
> + }
> +
> + return 0;
> +}
> +
> +int set_page_blks_state(struct page *page, unsigned long blk_states,
> + u64 start, u64 end)
> +{
> + return modify_page_blks_state(page, blk_states, start, end, 1);
> +}
> +
> +int clear_page_blks_state(struct page *page, unsigned long blk_states,
> + u64 start, u64 end)
> +{
> + return modify_page_blks_state(page, blk_states, start, end, 0);
> +}
> +
> +int test_page_blks_state(struct page *page, enum blk_state blk_state,
> + u64 start, u64 end, int check_all)
> +{
> + struct inode *inode = page->mapping->host;
> + unsigned long *bitmap;
> + unsigned long blk;
> + u64 nr_blks;
> + int found = 0;
> +
> + BUG_ON(!PagePrivate(page));
> +
> + bitmap = ((struct btrfs_page_private *)page->private)->bstate;
> +
> + blk = (start & (PAGE_CACHE_SIZE - 1)) >> inode->i_blkbits;
> + nr_blks = (end - start + 1) >> inode->i_blkbits;
> +
> + while (nr_blks--) {
> + if (test_bit((blk * BLK_NR_STATE) + blk_state, bitmap)) {
> + if (!check_all)
> + return 1;
> + found = 1;
> + } else if (check_all) {
> + return 0;
> + }
> +
> + ++blk;
> + }
> +
> + if (!check_all && !found)
> + return 0;
> +
> + return 1;
> +}
> +
> /*
> * either insert or lock state struct between start and end use mask to tell
> * us if waiting is desired.
> @@ -1982,14 +2064,22 @@ int test_range_bit(struct extent_io_tree *tree, u64 start, u64 end,
> * helper function to set a given page up to date if all the
> * extents in the tree for that page are up to date
> */
> -static void check_page_uptodate(struct extent_io_tree *tree, struct page *page)
> +static void check_page_uptodate(struct page *page)
> {
> u64 start = page_offset(page);
> u64 end = start + PAGE_CACHE_SIZE - 1;
> - if (test_range_bit(tree, start, end, EXTENT_UPTODATE, 1, NULL))
> + if (test_page_blks_state(page, BLK_STATE_UPTODATE, start, end, 1))
> SetPageUptodate(page);
> }
>
> +static int page_read_complete(struct page *page)
> +{
> + u64 start = page_offset(page);
> + u64 end = start + PAGE_CACHE_SIZE - 1;
> +
> + return !test_page_blks_state(page, BLK_STATE_IO, start, end, 0);
> +}
> +
> int free_io_failure(struct inode *inode, struct io_failure_record *rec)
> {
> int ret;
> @@ -2311,7 +2401,9 @@ int btrfs_check_repairable(struct inode *inode, struct bio *failed_bio,
> * a) deliver good data to the caller
> * b) correct the bad sectors on disk
> */
> - if (failed_bio->bi_vcnt > 1) {
> + if ((failed_bio->bi_vcnt > 1)
> + || (failed_bio->bi_io_vec->bv_len
> + > BTRFS_I(inode)->root->sectorsize)) {
> /*
> * to fulfill b), we need to know the exact failing sectors, as
> * we don't want to rewrite any more than the failed ones. thus,
> @@ -2520,18 +2612,6 @@ static void end_bio_extent_writepage(struct bio *bio, int err)
> bio_put(bio);
> }
>
> -static void
> -endio_readpage_release_extent(struct extent_io_tree *tree, u64 start, u64 len,
> - int uptodate)
> -{
> - struct extent_state *cached = NULL;
> - u64 end = start + len - 1;
> -
> - if (uptodate && tree->track_uptodate)
> - set_extent_uptodate(tree, start, end, &cached, GFP_ATOMIC);
> - unlock_extent_cached(tree, start, end, &cached, GFP_ATOMIC);
> -}
> -
> /*
> * after a readpage IO is done, we need to:
> * clear the uptodate bits on error
> @@ -2548,14 +2628,16 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
> struct bio_vec *bvec;
> int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
> struct btrfs_io_bio *io_bio = btrfs_io_bio(bio);
> + struct extent_state *cached = NULL;
> + struct btrfs_page_private *pg_private;
> struct extent_io_tree *tree;
> + unsigned long flags;
> u64 offset = 0;
> u64 start;
> u64 end;
> - u64 len;
> - u64 extent_start = 0;
> - u64 extent_len = 0;
> + int nr_sectors;
> int mirror;
> + int unlock;
> int ret;
> int i;
>
> @@ -2565,54 +2647,31 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
> bio_for_each_segment_all(bvec, bio, i) {
> struct page *page = bvec->bv_page;
> struct inode *inode = page->mapping->host;
> + struct btrfs_root *root = BTRFS_I(inode)->root;
>
> pr_debug("end_bio_extent_readpage: bi_sector=%llu, err=%d, "
> "mirror=%u\n", (u64)bio->bi_iter.bi_sector, err,
> io_bio->mirror_num);
> tree = &BTRFS_I(inode)->io_tree;
>
> - /* We always issue full-page reads, but if some block
> - * in a page fails to read, blk_update_request() will
> - * advance bv_offset and adjust bv_len to compensate.
> - * Print a warning for nonzero offsets, and an error
> - * if they don't add up to a full page. */
> - if (bvec->bv_offset || bvec->bv_len != PAGE_CACHE_SIZE) {
> - if (bvec->bv_offset + bvec->bv_len != PAGE_CACHE_SIZE)
> - btrfs_err(BTRFS_I(page->mapping->host)->root->fs_info,
> - "partial page read in btrfs with offset %u and length %u",
> - bvec->bv_offset, bvec->bv_len);
> - else
> - btrfs_info(BTRFS_I(page->mapping->host)->root->fs_info,
> - "incomplete page read in btrfs with offset %u and "
> - "length %u",
> - bvec->bv_offset, bvec->bv_len);
> - }
> -
> - start = page_offset(page);
> - end = start + bvec->bv_offset + bvec->bv_len - 1;
> - len = bvec->bv_len;
> -
> + start = page_offset(page) + bvec->bv_offset;
> + end = start + bvec->bv_len - 1;
> + nr_sectors = bvec->bv_len >> inode->i_sb->s_blocksize_bits;
> mirror = io_bio->mirror_num;
> - if (likely(uptodate && tree->ops &&
> - tree->ops->readpage_end_io_hook)) {
> +
> +next_block:
> + if (likely(uptodate)) {
Any reason of killing (tree->ops && tree->ops->readpage_end_io_hook)?
> ret = tree->ops->readpage_end_io_hook(io_bio, offset,
> - page, start, end,
> - mirror);
> + page, start,
> + start + root->sectorsize - 1,
> + mirror);
> if (ret)
> uptodate = 0;
> else
> clean_io_failure(inode, start, page, 0);
> }
>
> - if (likely(uptodate))
> - goto readpage_ok;
> -
> - if (tree->ops && tree->ops->readpage_io_failed_hook) {
> - ret = tree->ops->readpage_io_failed_hook(page, mirror);
> - if (!ret && !err &&
> - test_bit(BIO_UPTODATE, &bio->bi_flags))
> - uptodate = 1;
> - } else {
> + if (!uptodate) {
> /*
> * The generic bio_readpage_error handles errors the
> * following way: If possible, new read requests are
> @@ -2623,61 +2682,63 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
> * can't handle the error it will return -EIO and we
> * remain responsible for that page.
> */
> - ret = bio_readpage_error(bio, offset, page, start, end,
> - mirror);
> + ret = bio_readpage_error(bio, offset, page,
> + start, start + root->sectorsize - 1,
> + mirror);
> if (ret == 0) {
> - uptodate =
> - test_bit(BIO_UPTODATE, &bio->bi_flags);
> + uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
> if (err)
> uptodate = 0;
> - offset += len;
> - continue;
> + offset += root->sectorsize;
> + if (--nr_sectors) {
> + start += root->sectorsize;
> + goto next_block;
> + } else {
> + continue;
> + }
> }
> }
> -readpage_ok:
> - if (likely(uptodate)) {
> - loff_t i_size = i_size_read(inode);
> - pgoff_t end_index = i_size >> PAGE_CACHE_SHIFT;
> - unsigned off;
> -
> - /* Zero out the end if this page straddles i_size */
> - off = i_size & (PAGE_CACHE_SIZE-1);
> - if (page->index == end_index && off)
> - zero_user_segment(page, off, PAGE_CACHE_SIZE);
> - SetPageUptodate(page);
> +
> + if (uptodate) {
> + set_page_blks_state(page, 1 << BLK_STATE_UPTODATE, start,
> + start + root->sectorsize - 1);
> + check_page_uptodate(page);
> } else {
> ClearPageUptodate(page);
> SetPageError(page);
> }
> - unlock_page(page);
> - offset += len;
> -
> - if (unlikely(!uptodate)) {
> - if (extent_len) {
> - endio_readpage_release_extent(tree,
> - extent_start,
> - extent_len, 1);
> - extent_start = 0;
> - extent_len = 0;
> - }
> - endio_readpage_release_extent(tree, start,
> - end - start + 1, 0);
> - } else if (!extent_len) {
> - extent_start = start;
> - extent_len = end + 1 - start;
> - } else if (extent_start + extent_len == start) {
> - extent_len += end + 1 - start;
> - } else {
> - endio_readpage_release_extent(tree, extent_start,
> - extent_len, uptodate);
> - extent_start = start;
> - extent_len = end + 1 - start;
> +
> + offset += root->sectorsize;
> +
> + if (--nr_sectors) {
> + clear_page_blks_state(page, 1 << BLK_STATE_IO,
> + start, start + root->sectorsize - 1);
private->io_lock is not acquired here but not in below.
IIUC, this can be protected by EXTENT_LOCKED.
Thanks,
-liubo
> + clear_extent_bit(tree, start, start + root->sectorsize - 1,
> + EXTENT_LOCKED, 1, 0, &cached, GFP_ATOMIC);
> + start += root->sectorsize;
> + goto next_block;
> }
> +
> + WARN_ON(!PagePrivate(page));
> +
> + pg_private = (struct btrfs_page_private *)page->private;
> +
> + spin_lock_irqsave(&pg_private->io_lock, flags);
> +
> + clear_page_blks_state(page, 1 << BLK_STATE_IO,
> + start, start + root->sectorsize - 1);
> +
> + unlock = page_read_complete(page);
> +
> + spin_unlock_irqrestore(&pg_private->io_lock, flags);
> +
> + clear_extent_bit(tree, start, start + root->sectorsize - 1,
> + EXTENT_LOCKED, 1, 0, &cached, GFP_ATOMIC);
> +
> + if (unlock)
> + unlock_page(page);
> }
>
> - if (extent_len)
> - endio_readpage_release_extent(tree, extent_start, extent_len,
> - uptodate);
> if (io_bio->end_io)
> io_bio->end_io(io_bio, err);
> bio_put(bio);
> @@ -2859,13 +2920,36 @@ static void attach_extent_buffer_page(struct extent_buffer *eb,
> }
> }
>
> -void set_page_extent_mapped(struct page *page)
> +int set_page_extent_mapped(struct page *page)
> {
> + struct btrfs_page_private *pg_private;
> +
> if (!PagePrivate(page)) {
> + pg_private = kzalloc(sizeof(*pg_private), GFP_NOFS);
> + if (!pg_private)
> + return -ENOMEM;
> +
> + spin_lock_init(&pg_private->io_lock);
> +
> SetPagePrivate(page);
> page_cache_get(page);
> - set_page_private(page, EXTENT_PAGE_PRIVATE);
> +
> + set_page_private(page, (unsigned long)pg_private);
> + }
> +
> + return 0;
> +}
> +
> +int clear_page_extent_mapped(struct page *page)
> +{
> + if (PagePrivate(page)) {
> + kfree((struct btrfs_page_private *)(page->private));
> + ClearPagePrivate(page);
> + set_page_private(page, 0);
> + page_cache_release(page);
> }
> +
> + return 0;
> }
>
> static struct extent_map *
> @@ -2909,6 +2993,7 @@ static int __do_readpage(struct extent_io_tree *tree,
> unsigned long *bio_flags, int rw)
> {
> struct inode *inode = page->mapping->host;
> + struct extent_state *cached = NULL;
> u64 start = page_offset(page);
> u64 page_end = start + PAGE_CACHE_SIZE - 1;
> u64 end;
> @@ -2964,8 +3049,8 @@ static int __do_readpage(struct extent_io_tree *tree,
> memset(userpage + pg_offset, 0, iosize);
> flush_dcache_page(page);
> kunmap_atomic(userpage);
> - set_extent_uptodate(tree, cur, cur + iosize - 1,
> - &cached, GFP_NOFS);
> + set_page_blks_state(page, 1 << BLK_STATE_UPTODATE, cur,
> + cur + iosize - 1);
> if (!parent_locked)
> unlock_extent_cached(tree, cur,
> cur + iosize - 1,
> @@ -3017,8 +3102,8 @@ static int __do_readpage(struct extent_io_tree *tree,
> flush_dcache_page(page);
> kunmap_atomic(userpage);
>
> - set_extent_uptodate(tree, cur, cur + iosize - 1,
> - &cached, GFP_NOFS);
> + set_page_blks_state(page, 1 << BLK_STATE_UPTODATE, cur,
> + cur + iosize - 1);
> unlock_extent_cached(tree, cur, cur + iosize - 1,
> &cached, GFP_NOFS);
> cur = cur + iosize;
> @@ -3026,9 +3111,9 @@ static int __do_readpage(struct extent_io_tree *tree,
> continue;
> }
> /* the get_extent function already copied into the page */
> - if (test_range_bit(tree, cur, cur_end,
> - EXTENT_UPTODATE, 1, NULL)) {
> - check_page_uptodate(tree, page);
> + if (test_page_blks_state(page, BLK_STATE_UPTODATE, cur,
> + cur_end, 1)) {
> + check_page_uptodate(page);
> if (!parent_locked)
> unlock_extent(tree, cur, cur + iosize - 1);
> cur = cur + iosize;
> @@ -3048,6 +3133,9 @@ static int __do_readpage(struct extent_io_tree *tree,
> }
>
> pnr -= page->index;
> +
> + set_page_blks_state(page, 1 << BLK_STATE_IO, cur,
> + cur + iosize - 1);
> ret = submit_extent_page(rw, tree, page,
> sector, disk_io_size, pg_offset,
> bdev, bio, pnr,
> @@ -3059,8 +3147,11 @@ static int __do_readpage(struct extent_io_tree *tree,
> *bio_flags = this_bio_flag;
> } else {
> SetPageError(page);
> + clear_page_blks_state(page, 1 << BLK_STATE_IO, cur,
> + cur + iosize - 1);
> if (!parent_locked)
> - unlock_extent(tree, cur, cur + iosize - 1);
> + unlock_extent_cached(tree, cur, cur + iosize - 1,
> + &cached, GFP_NOFS);
> }
> cur = cur + iosize;
> pg_offset += iosize;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index c668f36..541b40a 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -51,11 +51,22 @@
> #define PAGE_SET_PRIVATE2 (1 << 4)
> #define PAGE_SET_ERROR (1 << 5)
>
> +enum blk_state {
> + BLK_STATE_UPTODATE,
> + BLK_STATE_DIRTY,
> + BLK_STATE_IO,
> + BLK_NR_STATE,
> +};
> +
> /*
> - * page->private values. Every page that is controlled by the extent
> - * map has page->private set to one.
> - */
> -#define EXTENT_PAGE_PRIVATE 1
> + The maximum number of blocks per page (i.e. 32) occurs when using 2k
> + as the block size and having 64k as the page size.
> +*/
> +#define BLK_STATE_NR_LONGS DIV_ROUND_UP(BLK_NR_STATE * 32, BITS_PER_LONG)
> +struct btrfs_page_private {
> + spinlock_t io_lock;
> + unsigned long bstate[BLK_STATE_NR_LONGS];
> +};
>
> struct extent_state;
> struct btrfs_root;
> @@ -259,7 +270,14 @@ int extent_readpages(struct extent_io_tree *tree,
> int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> __u64 start, __u64 len, get_extent_t *get_extent);
> int get_state_private(struct extent_io_tree *tree, u64 start, u64 *private);
> -void set_page_extent_mapped(struct page *page);
> +int set_page_extent_mapped(struct page *page);
> +int clear_page_extent_mapped(struct page *page);
> +int set_page_blks_state(struct page *page, unsigned long blk_states,
> + u64 start, u64 end);
> +int clear_page_blks_state(struct page *page, unsigned long blk_states,
> + u64 start, u64 end);
> +int test_page_blks_state(struct page *page, enum blk_state blk_state,
> + u64 start, u64 end, int check_all);
>
> struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> u64 start);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0020b56..8262f83 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6622,7 +6622,6 @@ struct extent_map *btrfs_get_extent(struct inode *inode, struct page *page,
> struct btrfs_key found_key;
> struct extent_map *em = NULL;
> struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
> - struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> struct btrfs_trans_handle *trans = NULL;
> const bool new_inline = !page || create;
>
> @@ -6800,8 +6799,8 @@ next:
> kunmap(page);
> btrfs_mark_buffer_dirty(leaf);
> }
> - set_extent_uptodate(io_tree, em->start,
> - extent_map_end(em) - 1, NULL, GFP_NOFS);
> + set_page_blks_state(page, 1 << BLK_STATE_UPTODATE, em->start,
> + extent_map_end(em) - 1);
> goto insert;
> }
> not_found:
> @@ -8392,11 +8391,9 @@ static int __btrfs_releasepage(struct page *page, gfp_t gfp_flags)
> tree = &BTRFS_I(page->mapping->host)->io_tree;
> map = &BTRFS_I(page->mapping->host)->extent_tree;
> ret = try_release_extent_mapping(map, tree, page, gfp_flags);
> - if (ret == 1) {
> - ClearPagePrivate(page);
> - set_page_private(page, 0);
> - page_cache_release(page);
> - }
> + if (ret == 1)
> + clear_page_extent_mapped(page);
> +
> return ret;
> }
>
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
