From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cuda.sgi.com (cuda2.sgi.com [192.48.176.25]) by oss.sgi.com (8.14.3/8.14.3/SuSE Linux 0.8) with ESMTP id q6V83Klb159081 for ; Tue, 31 Jul 2012 03:03:20 -0500 Received: from rcsinet15.oracle.com (rcsinet15.oracle.com [148.87.113.117]) by cuda.sgi.com with ESMTP id PK1WMFN4nCQM3ys9 (version=TLSv1 cipher=AES256-SHA bits=256 verify=NO) for ; Tue, 31 Jul 2012 01:03:19 -0700 (PDT) Message-ID: <5017913D.50707@oracle.com> Date: Tue, 31 Jul 2012 16:03:09 +0800 From: Jie Liu MIME-Version: 1.0 Subject: Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache. References: <50110630.7020902@oracle.com> <50116322.2020005@oracle.com> <20120731000606.GM2877@dastard> In-Reply-To: <20120731000606.GM2877@dastard> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: xfs-bounces@oss.sgi.com Errors-To: xfs-bounces@oss.sgi.com To: Dave Chinner Cc: xfs@oss.sgi.com On 07/31/12 08:06, Dave Chinner wrote: > On Thu, Jul 26, 2012 at 11:32:50PM +0800, Jeff Liu wrote: >> Search data buffer offset for given range from page cache. >> >> Signed-off-by: Jie Liu >> Reviewed-by: Mark Tinguely >> Reviewed-by: Christoph Hellwig >> Reviewed-by: Dave Chinner >> >> --- >> fs/xfs/xfs_file.c | 88 ++++++++++++++++++++++++++++++++++++++++++----------- >> 1 files changed, 70 insertions(+), 18 deletions(-) >> >> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c >> index 69965a4..b1158b3 100644 >> --- a/fs/xfs/xfs_file.c >> +++ b/fs/xfs/xfs_file.c >> @@ -1182,8 +1182,6 @@ xfs_seek_data( >> struct inode *inode = file->f_mapping->host; >> struct xfs_inode *ip = XFS_I(inode); >> struct xfs_mount *mp = ip->i_mount; >> - struct xfs_bmbt_irec map[2]; >> - int nmap = 2; >> loff_t uninitialized_var(offset); >> xfs_fsize_t isize; >> xfs_fileoff_t fsbno; >> @@ -1199,34 +1197,88 @@ xfs_seek_data( >> goto out_unlock; >> } >> >> - fsbno = XFS_B_TO_FSBT(mp, start); >> - >> /* >> * Try to read extents from the first block indicated >> * by fsbno to the end block of the file. >> */ >> + fsbno = XFS_B_TO_FSBT(mp, start); >> end = XFS_B_TO_FSB(mp, isize); >> + for (;;) { >> + struct xfs_bmbt_irec map[2]; >> + int nmap = 2; >> >> - error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap, >> - XFS_BMAPI_ENTIRE); >> - if (error) >> - goto out_unlock; >> + error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap, >> + XFS_BMAPI_ENTIRE); >> + if (error) >> + goto out_unlock; >> >> - /* >> - * Treat unwritten extent as data extent since it might >> - * contains dirty data in page cache. >> - */ >> - if (map[0].br_startblock != HOLESTARTBLOCK) { >> - offset = max_t(loff_t, start, >> - XFS_FSB_TO_B(mp, map[0].br_startoff)); >> - } else { >> - if (nmap == 1) { >> + /* No extents at given offset, must be beyond EOF */ >> + if (nmap == 0) { >> error = ENXIO; >> goto out_unlock; >> } >> >> offset = max_t(loff_t, start, >> - XFS_FSB_TO_B(mp, map[1].br_startoff)); >> + XFS_FSB_TO_B(mp, map[0].br_startoff)); >> + if (map[0].br_state == XFS_EXT_NORM && >> + !isnullstartblock(map[0].br_startblock)) >> + break; >> + else { >> + /* >> + * Landed in an unwritten extent, try to lookup data > Not correct - hole, delay or unwritten land here. Will fix it. > >> + * buffer from the page cache before proceeding to >> + * check the next extent map. It's a hole if nothing >> + * was found. >> + */ >> + if (map[0].br_startblock == DELAYSTARTBLOCK || >> + map[0].br_state == XFS_EXT_UNWRITTEN) { >> + /* Probing page cache start from offset */ >> + if (xfs_find_get_desired_pgoff(inode, &map[0], >> + DATA_OFF, &offset)) >> + break; >> + } > If is is a DELAYSTARTBLOCK, and it is within EOF, then it is > guaranteed to contain data. There is no need for a page cache lookup > to decide where the data starts - by definition the data starts at > map[0].br_startblock. I think you can treat DELAYSTARTBLOCK exactly > the same as allocated XFS_EXT_NORM extents. > > That means the logic is: > > if (map[0].br_state == XFS_EXT_NORM && > (!isnullstartblock(map[0].br_startblock) || > map[0].br_startblock == DELAYSTARTBLOCK)) { > break; > } else if (map[0].br_state == XFS_EXT_UNWRITTEN) { > /* Probing page cache start from offset */ > if (xfs_find_get_desired_pgoff(inode, &map[0], > DATA_OFF, &offset)) > break; > } else { > /* > * If we find a hole in map[0] and nothing in map[1] it > * probably means that we are reading after EOF. > */ > ..... > } > > Which kills a level of indentation in the code.... Just done a quick test with above modifications, it works to me, thanks. > > >> + /* >> + * Found a hole in map[0] and nothing in map[1]. >> + * Probably means that we are reading after EOF. >> + */ >> + if (nmap == 1) { >> + error = ENXIO; >> + goto out_unlock; >> + } >> + >> + /* >> + * We have two mappings, proceed to check map[1]. >> + */ >> + offset = max_t(loff_t, start, >> + XFS_FSB_TO_B(mp, map[1].br_startoff)); >> + if (map[1].br_state == XFS_EXT_NORM && >> + !isnullstartblock(map[1].br_startblock)) >> + break; >> + else { >> + /* >> + * map[1] is also an unwritten extent, lookup >> + * data buffer from page cache now. >> + */ >> + if (map[1].br_startblock == DELAYSTARTBLOCK || >> + map[1].br_state == XFS_EXT_UNWRITTEN) { >> + if (xfs_find_get_desired_pgoff(inode, >> + &map[1], DATA_OFF, &offset)) >> + break; >> + } >> + } > And the if/elseif/else logic above can be repeated here. > >> + } >> + >> + /* >> + * Nothing was found, proceed to the next round of search if >> + * reading offset not beyond or hit EOF. >> + */ >> + fsbno = map[1].br_startoff + map[1].br_blockcount; >> + start = XFS_FSB_TO_B(mp, fsbno); >> + if (start >= isize) { >> + error = ENXIO; >> + goto out_unlock; >> + } > Shouldn't this check be done at the start of the loop? If put this check at the beginning of the loop, the code block would looks like below: isize = i_size_read(inode); if (start >= isize) { error = ENXIO; goto out_unlock; } ....... for (;;) { struct xfs_bmbt_irec map[2]; int nmap = 2; if (start >= isize) { error = ENXIO; goto out_unlock; } error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap, XFS_BMAPI_ENTIRE); if (error) goto out_unlock; /* No extents at given offset, must be beyond EOF */ if (nmap == 0) { error = ENXIO; goto out_unlock; } .......... ..... /* * Nothing was found, proceed to the next round of search if * reading offset not beyond or hit EOF. */ fsbno = map[1].br_startoff + map[1].br_blockcount; start = XFS_FSB_TO_B(mp, fsbno); } But we have a similar check up at the begnning of xfs_seek_data(), will it looks a bit weird? Thanks, -Jeff > > Cheers, > > Dave. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs