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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
