Re: [PATCH] Btrfs: fix off-by-one in lseek

[Liu Bo <bo.li.liu@oracle.com>]

On Tue, Jan 08, 2013 at 06:26:27PM +0100, David Sterba wrote:
> On Tue, Jan 08, 2013 at 09:30:54AM +0100, David Sterba wrote:
> > On Tue, Jan 08, 2013 at 10:46:38AM +0800, Liu Bo wrote:
> > > Hmm, not always here, lockend = inode->i_size - 1, so lockend % 2 == 1 may not
> > > be true.
> > 
> > Yeah, that's the case that must be handled,
> > 
> > > But this check can worked in those places where we've rounded the range up to
> > > PAGE_SIZE :)
> > 
> > context:
> > 2233         u64 lockend = i_size_read(inode);
> > ...
> > 2236         u64 len = i_size_read(inode);
> > ...
> > 2240         lockend = max_t(u64, root->sectorsize, lockend);
> > ^^^^
> > 
> > lockend will be always at least root->sectorsize == PAGE_SIZE, so the simple
> > % 2 == 1 test should work generally. Am I missing something? Do we really lock
> > sub-PAGE_SIZE ranges?
> 
> Of course this only works for files smaller than PAGE_SIZE, a larger
> file is locked up to it's i_size. In that case the value of lockend that
> arrives at eg. __set_extent_bit can be of any value.
> We can access the inode->i_size via extent_io_tree::mapping->host, so
> it's possible to do the exact check.
> 
> with this simple check:
> 
> void check_range(const char *caller, struct inode *inode, u64 start, u64 end)
> {
>        u64 isize = i_size_read(inode);
> 
>        if (end >= PAGE_SIZE && (end % 2) == 0 && end != isize - 1) {
>                printk_ratelimited(KERN_DEBUG
>                        "D: %s isize = %llu odd range [%llu,%llu)\n",
>                                caller,
>                                (unsigned long long)isize,
>                                (unsigned long long)start,
>                                (unsigned long long)end);
>        }
> 
> }
> 
> called as:
> 
> 	check_range(__func__, tree->mapping->host, start, end);
> 
> from clear_extent_bit, wait_extent_bit, __set_extent_bit,
> convert_extent_bit and your "lockend--" fix I can't reproduce the
> off-by-one error.
> 
> $ cat seektest.c
> #include <stdlib.h>
> #include <stdio.h>
> #include <unistd.h>
> #include <string.h>
> #include <sys/fcntl.h>
> 
> #ifndef SEEK_DATA
> #define SEEK_DATA 3
> #endif
> 
> static char buf[16*1024];
> 
> void do_test(const char *fname, loff_t data_start, loff_t len) {
>         int fd;
>         loff_t lret;
> 
>         fd = open(fname, O_RDWR|O_CREAT, 0644);
>         if (fd == -1) {
>                 perror("open()");
>                 exit(1);
>         }
>         printf("[0,%llu] hole\n", data_start - 1);
>         llseek(fd, data_start, SEEK_SET);
>         write(fd, buf, len);
>         printf("[%llu,%llu] data\n", data_start, len - 1);
>         fsync(fd);
>         llseek(fd, 0, SEEK_SET);
>         lret = llseek(fd, 0, SEEK_DATA);
>         printf("seek to data returned: %llu\n",
>                         (unsigned long long)lret);
>         close(fd);
> }
> 
> int main() {
>         do_test("seektest-small-even", 3072, 512);
>         do_test("seektest-small-odd", 3072, 511);
>         do_test("seektest-big-even", 8192, 512);
>         do_test("seektest-big-odd", 8192, 511);
>         do_test("seektest-big-aligned", 8192, 4096);
>         do_test("seektest-big-aligned-1", 8192, 4095);
>         do_test("seektest-big-aligned+1", 8192, 4097);
> 
>         return 0;
> }
> ---
> 
> However, I'm running xfstests with this debugging patch and I cant'
> explain such messages:
> 
> [20177.655732] D: __set_extent_bit isize = 0 odd range [352256,8740681790172090368)
> [20177.664485] D: clear_extent_bit isize = 0 odd range [352256,8740681790172090368)
> [20182.399257] D: __set_extent_bit isize = 944560 odd range [229376,7504638559450173440)
> [20182.408465] D: clear_extent_bit isize = 944560 odd range [229376,7504638559450173440)
> [20183.543186] D: __set_extent_bit isize = 396355 odd range [692224,2301956497953013760)
> [20183.552170] D: clear_extent_bit isize = 396355 odd range [692224,2301956497953013760)
> [20187.018092] D: __set_extent_bit isize = 0 odd range [401408,306555945915797504)
> [20187.026532] D: clear_extent_bit isize = 0 odd range [401408,306555945915797504)
> [20189.941615] D: __set_extent_bit isize = 1936258 odd range [2457600,6817576374066888704)
> [20189.950751] D: clear_extent_bit isize = 1936258 odd range [2457600,6817576374066888704)
> [20190.642796] D: __set_extent_bit isize = 0 odd range [274432,8022548162794254336)
> [20190.651380] D: clear_extent_bit isize = 0 odd range [274432,8022548162794254336)
> [20191.244458] D: __set_extent_bit isize = 845491 odd range [1671168,5821247760915324928)
> [20191.253508] D: clear_extent_bit isize = 845491 odd range [1671168,5821247760915324928)
> [20191.948060] D: __set_extent_bit isize = 0 odd range [774144,7384799041917984768)
> [20191.956581] D: clear_extent_bit isize = 0 odd range [774144,7384799041917984768)
> 
> so I'm not sending it as a separate patch yet until the check covers all cases.

Thanks for coding this up, I've checked the code, these messages can
be fixed by the following, please check if it works on your side :)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1b319df..1688669 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3895,7 +3913,7 @@ int extent_fiemap(struct inode *inode, struct
fiemap_extent_info *fieinfo,
 		last_for_get_extent = isize;
 	}
 
-	lock_extent_bits(&BTRFS_I(inode)->io_tree, start, start + len, 0,
+	lock_extent_bits(&BTRFS_I(inode)->io_tree, start, start + len - 1, 0,
 			 &cached_state);
 
 	em = get_extent_skip_holes(inode, start, last_for_get_extent,
@@ -3982,7 +4000,7 @@ int extent_fiemap(struct inode *inode, struct
fiemap_extent_info *fieinfo,
 out_free:
 	free_extent_map(em);
 out:
-	unlock_extent_cached(&BTRFS_I(inode)->io_tree, start, start + len,
+	unlock_extent_cached(&BTRFS_I(inode)->io_tree, start, start + len - 1,
 			     &cached_state, GFP_NOFS);
 	return ret;
 }

thanks,
liubo
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo