From patchwork Wed Jan 9 04:34:45 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Liu Bo X-Patchwork-Id: 1950501 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 60707DF2EB for ; Wed, 9 Jan 2013 04:37:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756303Ab3AIEht (ORCPT ); Tue, 8 Jan 2013 23:37:49 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:34215 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753589Ab3AIEhs (ORCPT ); Tue, 8 Jan 2013 23:37:48 -0500 Received: from acsinet22.oracle.com (acsinet22.oracle.com [141.146.126.238]) by aserp1040.oracle.com (Sentrion-MTA-4.2.2/Sentrion-MTA-4.2.2) with ESMTP id r094b4tK011889 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 9 Jan 2013 04:37:05 GMT Received: from acsmt358.oracle.com (acsmt358.oracle.com [141.146.40.158]) by acsinet22.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id r094b4oT028479 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 9 Jan 2013 04:37:04 GMT Received: from abhmt109.oracle.com (abhmt109.oracle.com [141.146.116.61]) by acsmt358.oracle.com (8.12.11.20060308/8.12.11) with ESMTP id r094b4S3030052; Tue, 8 Jan 2013 22:37:04 -0600 Received: from liubo (/219.145.43.160) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 08 Jan 2013 20:37:02 -0800 Date: Wed, 9 Jan 2013 12:34:45 +0800 From: Liu Bo To: David Sterba Cc: linux-btrfs@vger.kernel.org Subject: Re: [PATCH] Btrfs: fix off-by-one in lseek Message-ID: <20130109043443.GA4527@liubo> Reply-To: bo.li.liu@oracle.com References: <1357530788-5790-1-git-send-email-bo.li.liu@oracle.com> <20130107162050.GJ20089@twin.jikos.cz> <20130108024637.GA1916@liubo> <20130108083053.GD3763@twin.jikos.cz> <20130108172627.GP20089@twin.jikos.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130108172627.GP20089@twin.jikos.cz> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: acsinet22.oracle.com [141.146.126.238] Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org 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 > #include > #include > #include > #include > > #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 :) 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 Tested-by: David Sterba --- 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-info.html 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);