On 09/28/2011 04:53 PM, Yongqiang Yang wrote: > On Wed, Sep 28, 2011 at 4:40 PM, Jeff Liu <jeff.liu@xxxxxxxxxx> wrote: >> On 09/28/2011 04:11 PM, Yongqiang Yang wrote: >> >>> On Wed, Sep 28, 2011 at 4:02 PM, Jeff Liu <jeff.liu@xxxxxxxxxx> wrote: >>>> On 09/28/2011 03:21 PM, Yongqiang Yang wrote: >>>> >>>>> On Wed, Sep 28, 2011 at 2:32 PM, Jeff Liu <jeff.liu@xxxxxxxxxx> wrote: >>>>>> Hello all, >>>>>> >>>>>> This is the V2 for add SEEK_DATA/SEEK_HOLE support to ext4. >>>>>> In V1, I copied all the codes regarding fetching delayed-extents out to >>>>>> my seek callback function, it's stupid to duplicate the code blocks. >>>>>> >>>>>> Now the idea is, since the delayed-extents is the main concern to search >>>>>> extents for both fiemap and seek, so IMHO, it's better to consolidate >>>>>> this procedure into a new function(get_delayed_extent() in my patch), >>>>>> and call it in fiemap and seek call-back functions if necessary. In >>>>>> this way, when yongqiang's delayed-extents tree is ready, we only need >>>>>> to modify the code block at get_delayed_extent() accordingly. >>>>>> >>>>>> In addition, yongqiang has pointed out there might be a deadlock, but >>>>>> per my understood, as the target inode already locked via i_mutex in >>>>>> ext4_llseek() before, this lock can prevent any modification to the >>>>>> file, and we call find_get_pages_tag() when trying to fetch the dirty >>>>>> pages, this function using rcu read lock, it also safe to read. So I >>>>>> have no idea what's wrong, could anyone please kindly point me out in >>>>>> this case? I'm definitely a newbie to this list, sorry for my ignorant. >>>>> Ok. There is no deadlock, fiemap releases i_data_sem before calling >>>>> get_delayed_extent(). >>>>> >>>>> BTW: It seems that there is a bug in fiemap according to code, I am >>>>> not sure which commit introduced it, delayed extents beyond last >>>>> allocated block are ignored. >>>> >>>> Hmm... could you show me a bit more detailed info with the test scenario >>>> if convenient? I can help testing, at least 3.1.0-rc2+ is works to me. >>> Could you test a file with map info like AAAADDDDDHHHHDDDD, >>> A-allocated block, D-deayed block, H-hole. then filefrag -v and have >>> a look if extent map info is right. >> >> Do you means, open a file, write something, fsync(), then write >> something, seek to some position and write something again? > Yep. >> If so, looks the filefrag -v result is ok by waiting for a few seconds. > What do you mean waiting for a few seconds? If run filefrag -v immediately against a just created file, the delayed extents will not shown, we have to wait a little while until they are really allocated IMHO. Thanks, -Jeff > > > Yongqiang. >> >> Thanks, >> -Jeff >> >>> >>> Thanks, >>> Yongqiang. >>>> >>>> Also, upstream coreutils-cp(1) has been well tested for ext4, it could >>>> be used for fiemap tests based on the extent_scan module: >>>> http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/extent-scan.c?h=next&id=b56b53bd70b1f8fa2b5a95d4569bb72a2419b5cd >>>> >>>> Thanks, >>>> -Jeff >>>> >>>>> >>>>> Yongqiang. >>>>>> >>>>>> Anyway, I'd like to post the revised for your guys review first. >>>>>> In have split the modification to two patches this time, since I hope to >>>>>> make it looks more convenient for the review purpose. >>>>>> >>>>>> The 1st patch, remove the delayed-extents fetching procedure to a >>>>>> function named as get_delayed_extent(), and modify the >>>>>> ext4_ext_fiemap_cb() accordingly. >>>>>> >>>>>> The 2nd path add the ext4_ext_seek_cb() to implement the >>>>>> SEEK_HOLE/SEEK_DATA stuff based on previous code change. >>>>>> >>>>>> Testing: >>>>>> ======== >>>>>> In addition to the test cases mentioned in V1, I also done another tests >>>>>> through sunil's seek test program, please refer to: >>>>>> http://oss.oracle.com/~smushran/seek_data/seek_test.c >>>>>> >>>>>> To ensure the 1st patch does not cause regression issue on fiemap, I >>>>>> have done some tests via cp(1) with fiemap copy support, the sparse file >>>>>> used for this test is produced by: >>>>>> python -c "f=open('./sptest', 'w'); [(f.seek(x) or f.write(str(x))) for >>>>>> x in range(1, 1000000000, 99999)]; f.close() >>>>>> >>>>>> >>>>>> Any feedback are appreciated! >>>>>> >>>>>> >>>>>> Thanks, >>>>>> -Jeff >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>> >>> >>> >> >> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
