Re: Introduce SEEK_DATA/SEEK_HOLE to XFS V5

[Jeff Liu <jeff.liu@oracle.com>]

Hi Mark,

Thanks for your comments!

On 01/12/2012 05:07 AM, Mark Tinguely wrote:

> 
> xfs_bmapi_read() returns the br_state == XFS_EXT_NORM for a hole.

Yes, this is key point I have missed before.

> There are a couple places that a hole can trigger a data test.
> BTW, I could not generate a large enough hole that xfs_bmapi_read()
> would return as more than one hole entry, so I will ignore those
> situations and just list the couple places that a hole may be match
> a data rule:
> 
> in xfs_seek_data():
> +        /*
> +         * Landed in an unwritten extent, try to find out the data
> +         * buffer offset from page cache firstly. If nothing was
> +         * found, treat it as a hole, and skip to check the next
> +         * extent, something just like above.
> +         */
> +        if (map[0].br_state == XFS_EXT_UNWRITTEN) {
> +            if (xfs_has_unwritten_buffer(inode, &map[0],
> +                             PAGECACHE_TAG_DIRTY,
> +                             &offset) ||
> +                xfs_has_unwritten_buffer(inode, &map[0],
> +                             PAGECACHE_TAG_WRITEBACK,
> +                             &offset)) {
> +                offset = max_t(loff_t, seekoff, offset);
> +                break;
> +            }
> +
> +            /* No data extent at the given offset */
> +            if (nmap == 1) {
> +                error = ENXIO;
> +                break;
> +            }
> +
> +            if (map[1].br_state == XFS_EXT_NORM ||
>             ^^^ could be a hole and not data^^^
> 
> I think you need to add back the br_startblock test:
> 
> +            if ((map[1].br_state == XFS_EXT_NORM &&
> +                 map[1].br_startblock != HOLESTARTBLOCK) ||

Ok, I'll add !isnullstartblock() test for normal extents test.

> 
> 
> in xfs_seek_hole():
> +        /*
> +         * Landed in a delay allocated extent or a real data extent,
> +         * if the next extent is landed in a hole or in an unwritten
> +         * extent but without data committed in the page cache, return
> +         * its offset. If the next extent has dirty data in page cache,
> +         * but its offset starts past both the start block of the map
> +         * and the seek offset, it still be a hole.
> +         */
> +        if (map[0].br_startblock == DELAYSTARTBLOCK ||
> +            map[0].br_state == XFS_EXT_NORM) {
>             ^^^ could be a hole ^^^
> 
>    and this only matters because this test is checked before the next test:
>        
> +
> +        /* Landed in a hole, its fine to return */
> +        if (map[0].br_startblock == HOLESTARTBLOCK) {
> +            offset = max_t(loff_t, seekoff,
> +                       XFS_FSB_TO_B(mp, map[0].br_startoff));
> +            break;
> +        }
> 
> 
> 
> Switching the order of these two tests would return the immediate offset
> starting a hole seek at the offset of a hole.

looks this issue is caused by missing hole test for extents at
XFS_EXT_NORM state. I'll fix them later.

Thanks,
-Jeff

> 
> 
> None of these conditions will result in data corruption, only earlier
> detection of a hole.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs