On Thu, Feb 24, 2011 at 8:40 AM, Yongqiang Yang <xiaoqiangnk@xxxxxxxxx> wrote: > On Thu, Feb 24, 2011 at 12:41 AM, Eric Sandeen <sandeen@xxxxxxxxxx> wrote: >> On 2/23/11 9:59 AM, Yongqiang Yang wrote: >>> 1] Delayed extents after a hole are neglected. >>> >>> By using find_get_pages() instead of find_get_page() to >>> lookup pagecache, delayed extents can be found, because >>> find_get_pages() with nr_pages=1 will return the next page >>> in pagecache. >>> >>> 2] Extents after a delayed extent or a hole are neglected as well. >>> >>> Fix it by accurating the request range by the result of >>> ext4_ext_next_allocated_block(). >>> >>> Reported by Chris Mason <chris.mason@xxxxxxxxxx>: >>> We've had reports on btrfs that cp is giving us files full of zeros >>> instead of actually copying them. It was tracked down to a bug with >>> the btrfs fiemap implementation where it was returning holes for >>> delalloc ranges. >>> >>> Newer versions of cp are trusting fiemap to tell it where the holes >>> are, which does seem like a pretty neat trick. >>> >>> I decided to give xfs and ext4 a shot with a few tests cases too, xfs >>> passed with all the ones btrfs was getting wrong, and ext4 got the basic >>> delalloc case right. >>> $ mkfs.ext4 /dev/xxx >>> $ mount /dev/xxx /mnt >>> $ dd if=/dev/zero of=/mnt/foo bs=1M count=1 >>> $ fiemap-test foo >>> ext: 0 logical: [ 0.. 255] phys: 0.. 255 >>> flags: 0x007 tot: 256 >>> >>> Horray! But once we throw a hole in, things go bad: >>> $ mkfs.ext4 /dev/xxx >>> $ mount /dev/xxx /mnt >>> $ dd if=/dev/zero of=/mnt/foo bs=1M count=1 seek=1 >>> $ fiemap-test foo >>> < no output > >>> >>> We've got a delalloc extent after the hole and ext4 fiemap didn't find >>> it. If I run sync to kick the delalloc out: >>> $sync >>> $ fiemap-test foo >>> ext: 0 logical: [ 256.. 511] phys: 34048.. 34303 >>> flags: 0x001 tot: 256 >>> >>> fiemap-test is sitting in my /usr/local/bin, and I have no idea how it >>> got there. It's full of pretty comments so I know it isn't mine, but >>> you can grab it here: >>> >>> http://oss.oracle.com/~mason/fiemap-test.c >>> >>> xfsqa has a fiemap program too. >>> >>> After Fix, test results are as follows: >>> ext: 0 logical: [ 256.. 511] phys: 0.. 255 >>> flags: 0x007 tot: 256 >>> ext: 0 logical: [ 256.. 511] phys: 33280.. 33535 >>> flags: 0x001 tot: 256 >>> >>> Signe-off-by: Yongqiang Yang <xiaoqiangnk@xxxxxxxxx> >>> --- >>> fs/ext4/extents.c | 26 +++++++++++++++++++++++--- >>> mm/filemap.c | 1 + >>> 2 files changed, 24 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >>> index ccce8a7..ad455a0 100644 >>> --- a/fs/ext4/extents.c >>> +++ b/fs/ext4/extents.c >>> @@ -3788,17 +3788,27 @@ static int ext4_ext_fiemap_cb(struct inode *inode, struct ext4_ext_path *path, >>> __u64 physical; >>> __u64 length; >>> __u32 flags = 0; >>> + ext4_lblk_t end; >>> int error; >>> >>> logical = (__u64)newex->ec_block << blksize_bits; >>> >>> - if (newex->ec_start == 0) { >>> + if (!newex->ec_start) { >>> + /* >>> + * There is no extent contains @newex->ec_block block. >>> + * It implies that @newex->ec_block block lies 1)a hole >>> + * or 2)delayed-allocated blocks that has not been >>> + * allocated, so pagecache is needed to lookup. >>> + * >>> + * And if it is case 2, @newex->ec_len needs to be corrected. >>> + * >>> + */ >>> pgoff_t offset; >>> struct page *page; >>> struct buffer_head *bh = NULL; >>> >>> offset = logical >> PAGE_SHIFT; >>> - page = find_get_page(inode->i_mapping, offset); >>> + (void)find_get_pages(inode->i_mapping, offset, 1, &page); >>> if (!page || !page_has_buffers(page)) >>> return EXT_CONTINUE; >>> >>> @@ -3807,8 +3817,13 @@ static int ext4_ext_fiemap_cb(struct inode *inode, struct ext4_ext_path *path, >>> if (!bh) >>> return EXT_CONTINUE; >>> >>> + /* Assume block-size equals page-size. */ >>> if (buffer_delay(bh)) { >>> flags |= FIEMAP_EXTENT_DELALLOC; >>> + if (page->index > offset) { >>> + logical = ((__u64)page->index << PAGE_SHIFT); >>> + newex->ec_block = logical >> blksize_bits; >>> + } >>> page_cache_release(page); >>> } else { >>> page_cache_release(page); >>> @@ -3830,7 +3845,8 @@ static int ext4_ext_fiemap_cb(struct inode *inode, struct ext4_ext_path *path, >>> * >>> * XXX this might miss a single-block extent at EXT_MAX_BLOCK >>> */ >>> - if (ext4_ext_next_allocated_block(path) == EXT_MAX_BLOCK || >>> + end = ext4_ext_next_allocated_block(path); >> >> I think this will fall down if you have: >> >> [ HOLE ][ DELALLOC ][ HOLE ][ ALLOCATED ] won't it? >> >> i.e. your "end" will be the first block of "allocated" right? > We use pagevec_lookup_tag() instead of find_get_page() and check > BH_Delay of contiguous pages. Then, we can deal this model. > > How do you think? If we have a function which can get contiguous pages with specified tag, it will be greater! I am not sure if adding this function is allowed. >> >> -Eric >> >>> + if (end == EXT_MAX_BLOCK || >>> newex->ec_block + newex->ec_len - 1 == EXT_MAX_BLOCK) { >>> loff_t size = i_size_read(inode); >>> loff_t bs = EXT4_BLOCK_SIZE(inode->i_sb); >>> @@ -3839,8 +3855,12 @@ static int ext4_ext_fiemap_cb(struct inode *inode, struct ext4_ext_path *path, >>> if ((flags & FIEMAP_EXTENT_DELALLOC) && >>> logical+length > size) >>> length = (size - logical + bs - 1) & ~(bs-1); >>> + } else { >>> + newex->ec_len = end - newex->ec_block; >>> + length = (__u64)newex->ec_len << blksize_bits; >>> } >>> >>> + >>> error = fiemap_fill_next_extent(fieinfo, logical, physical, >>> length, flags); >>> if (error < 0) >>> diff --git a/mm/filemap.c b/mm/filemap.c >>> index 83a45d3..1c01ffc 100644 >>> --- a/mm/filemap.c >>> +++ b/mm/filemap.c >>> @@ -803,6 +803,7 @@ repeat: >>> rcu_read_unlock(); >>> return ret; >>> } >>> +EXPORT_SYMBOL(find_get_pages); >>> >>> /** >>> * find_get_pages_contig - gang contiguous pagecache lookup >> >> > > > > -- > Best Wishes > Yongqiang Yang > -- Best Wishes Yongqiang Yang -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
