xfs
[Top] [All Lists]

Re: Introduce SEEK_DATA/SEEK_HOLE to XFS V5

To: jeff.liu@xxxxxxxxxx
Subject: Re: Introduce SEEK_DATA/SEEK_HOLE to XFS V5
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Wed, 11 Jan 2012 15:07:29 -0600
Cc: Ben Myers <bpm@xxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, Chris Mason <chris.mason@xxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <4F0D21E5.7010908@xxxxxxxxxx>
References: <4F06F71A.2010301@xxxxxxxxxx> <20120110171855.GX6390@xxxxxxx> <4F0D21E5.7010908@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.24) Gecko/20111206 Thunderbird/3.1.16

xfs_bmapi_read() returns the br_state == XFS_EXT_NORM for a hole.
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) ||


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.


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

<Prev in Thread] Current Thread [Next in Thread>