From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josef Bacik Subject: Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester Date: Wed, 29 Jun 2011 09:19:41 -0400 Message-ID: <4E0B266D.30000@redhat.com> References: <1309275199-10801-1-git-send-email-josef@redhat.com> <1309275199-10801-5-git-send-email-josef@redhat.com> <20110629065306.GC1026@dastard> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org, xfs@oss.sgi.com, viro@ZenIV.linux.org.uk To: Dave Chinner Return-path: In-Reply-To: <20110629065306.GC1026@dastard> List-ID: On 06/29/2011 02:53 AM, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> This is a test to make sure seek_data/seek_hole is acting like it do= es on >> Solaris. It will check to see if the fs supports finding a hole or = not and will >> adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. > > - cold cache behaviour - all dirty data ranges the test > creates are hot in cache and not even forced to disk, so > it is not testing the no-page-cache-over-the-data-range > case. i.e. it tests delalloc state tracking but not > data-extent-already exists lookups during a seek. > > - assumes that allocation size is the block size and that > holes follows block size alignment. We already know that > ext4 does not follow that rule when doing small sparse > writes close together in a file, and XFS is also known to > fill holes when doing sparse writes past EOF. > > - only tests single block data extents =D1=95o doesn't cover > corner cases like skipping over multiple fragmented data > extents to the next hole. > Yeah I intentionally left out preallocated stuff because these are goin= g=20 to be implementation specific, so I was going to leave that for a later= =20 exercise when people actually start doing proper implementations. > Some more comments in line.... > >> +_cleanup() >> +{ >> + rm -f $tmp.* >> +} >> + >> +trap "_cleanup ; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +# real QA test starts here >> +_supported_fs generic >> +_supported_os Linux >> + >> +testfile=3D$TEST_DIR/seek_test.$$ >> +logfile=3D$TEST_DIR/seek_test.$$.log > > The log file is usually named $seq.full, and doesn't get placed in > the filesystem being tested. It gets saved in the xfstests directory > along side $seq.out.bad for analysis whenteh test fails... > I only want it to see if SEEK_HOLE fails so I can say it didn't run. I= =20 followed the same example as the fiemap test that Eric wrote. >> +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" >> + >> +_cleanup() >> +{ >> + rm -f $testfile >> + rm -f $logfile >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +echo "Silence is golden" >> +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile > > Personally I'd prefer the test to be a bit noisy about what it is > running, especially when there are so many subtests the single > invocation is running. It makes no difference to the run time ofthe > test, or the output when something fails, but it at least allows you > to run the test manually and see what it is doing easily... > Right, the problem with this test is it will run differently depending=20 on the implementation. I agree, I really like the noisy output tests,=20 but unfortunately if I run this test on ext4 where it currently treats=20 the entire file as data, and then run it on btrfs where it is smarter=20 and actually recognizes holes, we end up with two different outputs tha= t=20 are both correct. >> + >> +if grep -q "SEEK_HOLE is not supported" $logfile; then >> + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" >> +fi >> + >> +rm -f $logfile >> +rm -f $testfile >> + >> +status=3D0 ; exit >> diff --git a/255.out b/255.out >> new file mode 100644 >> index 0000000..7eefb82 >> --- /dev/null >> +++ b/255.out >> @@ -0,0 +1,2 @@ >> +QA output created by 255 >> +Silence is golden >> diff --git a/group b/group >> index 1f86075..c045e70 100644 >> --- a/group >> +++ b/group >> @@ -368,3 +368,4 @@ deprecated >> 252 auto quick prealloc >> 253 auto quick >> 254 auto quick >> +255 auto quick > > I'd suggest that rw and prealloc (once unwritten extent > testing is added) groups should also be defined for this test. > > Otherwise, the test code looks ok if a bit over-engineered.... > >> +struct testrec { >> + int test_num; >> + int (*test_func)(int fd, int testnum); >> + char *test_desc; >> +}; >> + >> +struct testrec seek_tests[] =3D { >> + { 1, test01, "Test basic support" }, >> + { 2, test02, "Test an empty file" }, >> + { 3, test03, "Test a full file" }, >> + { 4, test04, "Test file hole at beg, data at end" }, >> + { 5, test05, "Test file data at beg, hole at end" }, >> + { 6, test06, "Test file hole data hole data" }, > > So, to take from the hole punch test matrix, it covers a bunch more > file state transitions and cases that are just as relevant to > SEEK_HOLE/SEEK_DATA. Those cases are: > > # 1. into a hole > # 2. into allocated space > # 3. into unwritten space > # 4. hole -> data > # 5. hole -> unwritten > # 6. data -> hole > # 7. data -> unwritten > # 8. unwritten -> hole > # 9. unwritten -> data > # 10. hole -> data -> hole > # 11. data -> hole -> data > # 12. unwritten -> data -> unwritten > # 13. data -> unwritten -> data > # 14. data -> hole @ EOF > # 15. data -> hole @ 0 > # 16. data -> cache cold ->hole > # 17. data -> hole in single block file > > I thikn we also need to cover most of these same cases, right? And > SEEK_HOLE/SEEK data also need to explicitly separate the unwritten > tests into "clean unwritten" and "dirty unwritten" and cover the > transitions between regions of those states as well, right? > Yeah you are right, but again doing preallocated stuff is tricky, but I= =20 can expand it now if that's what we want. Thanks, Josef 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 p5TDKHHV102796 for ; Wed, 29 Jun 2011 08:20:17 -0500 Received: from mx1.redhat.com (localhost [127.0.0.1]) by cuda.sgi.com (Spam Firewall) with ESMTP id 3DA4840327 for ; Wed, 29 Jun 2011 06:19:52 -0700 (PDT) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by cuda.sgi.com with ESMTP id zHtEk7LVG5s122t7 for ; Wed, 29 Jun 2011 06:19:52 -0700 (PDT) Message-ID: <4E0B266D.30000@redhat.com> Date: Wed, 29 Jun 2011 09:19:41 -0400 From: Josef Bacik MIME-Version: 1.0 Subject: Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester References: <1309275199-10801-1-git-send-email-josef@redhat.com> <1309275199-10801-5-git-send-email-josef@redhat.com> <20110629065306.GC1026@dastard> In-Reply-To: <20110629065306.GC1026@dastard> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: base64 Content-Type: text/plain; charset="utf-8"; Format="flowed" Sender: xfs-bounces@oss.sgi.com Errors-To: xfs-bounces@oss.sgi.com To: Dave Chinner Cc: linux-fsdevel@vger.kernel.org, viro@ZenIV.linux.org.uk, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org, xfs@oss.sgi.com T24gMDYvMjkvMjAxMSAwMjo1MyBBTSwgRGF2ZSBDaGlubmVyIHdyb3RlOgo+IE9uIFR1ZSwgSnVu IDI4LCAyMDExIGF0IDExOjMzOjE5QU0gLTA0MDAsIEpvc2VmIEJhY2lrIHdyb3RlOgo+PiBUaGlz IGlzIGEgdGVzdCB0byBtYWtlIHN1cmUgc2Vla19kYXRhL3NlZWtfaG9sZSBpcyBhY3RpbmcgbGlr ZSBpdCBkb2VzIG9uCj4+IFNvbGFyaXMuICBJdCB3aWxsIGNoZWNrIHRvIHNlZSBpZiB0aGUgZnMg c3VwcG9ydHMgZmluZGluZyBhIGhvbGUgb3Igbm90IGFuZCB3aWxsCj4+IGFkanVzdCBhcyBuZWNl c3NhcnkuCj4KPiBTbyBJIGp1c3QgbG9va2VkIGF0IHRoaXMgd2l0aCBhbiBleWUgdG8gdmFsaWRh dGluZyBhbiBYRlMKPiBpbXBsZW1lbnRhdGlvbiwgYW5kIEkgY2FtZSB1cCB3aXRoIHRoaXMgbGlz dCBvZiBzdHVmZiB0aGF0IHRoZSB0ZXN0Cj4gZG9lcyBub3QgY292ZXIgdGhhdCBJJ2QgbmVlZCB0 byB0ZXN0IGluIHNvbWUgd2F5Ogo+Cj4gCS0gZmlsZXMgd2l0aCBjbGVhbiB1bndyaXR0ZW4gZXh0 ZW50cy4gQXJlIHRoZXkgYSBob2xlIG9yCj4gCSAgZGF0YT8gV2hhdCdzIFNFRUtfREFUQSBzdXBw b3NlZCB0byByZXR1cm4gb24gbGF5b3V0IGxpa2UKPiAJICBob2xlLXVud3JpdHRlbi1kYXRhPyBp LmUuIG5lZWRzIHRvIGFkZCBmYWxsb2NhdGUgdG8gdGhlCj4gCSAgcGljdHVyZS4uLgo+Cj4gCS0g ZmlsZXMgd2l0aCBkaXJ0eSB1bndyaXR0ZW4gZXh0ZW50cyAoaS5lLiBkaXJ0eSBpbiBtZW1vcnks Cj4gCSAgbm90IG9uIGRpc2spLiBUaGV5IGFyZSBtb3N0IGRlZmluaXRlbHkgZGF0YSwgYW5kIG1v c3QKPiAJICBmaWxlc3lzdGVtcyB3aWxsIG5lZWQgYSBzZXBhcmF0ZSBsb29rdXAgcGF0aCB0byBk ZXRlY3QKPiAJICBkaXJ0eSB1bndyaXR0ZW4gcmFuZ2VzIGJlY2F1c2UgdGhlIHN0YXRlIGlzIGtl cHQKPiAJICBzZXBhcmF0ZWx5IChwYWdlIGNhY2hlIHZzIGV4dGVudCBjYWNoZSkuICBQbGVudHkg b2Ygc2NvcGUKPiAJICBmb3IgZmlsZXN5c3RlbSBzcGVjaWZpYyBidWdzIGhlcmUgc28gbmVlZHMg YSByb3VidXN0IHRlc3QuCj4KPiAJLSBjb2xkIGNhY2hlIGJlaGF2aW91ciAtIGFsbCBkaXJ0eSBk YXRhIHJhbmdlcyB0aGUgdGVzdAo+IAkgIGNyZWF0ZXMgYXJlIGhvdCBpbiBjYWNoZSBhbmQgbm90 IGV2ZW4gZm9yY2VkIHRvIGRpc2ssIHNvCj4gCSAgaXQgaXMgbm90IHRlc3RpbmcgdGhlIG5vLXBh Z2UtY2FjaGUtb3Zlci10aGUtZGF0YS1yYW5nZQo+IAkgIGNhc2UuIGkuZS4gaXQgdGVzdHMgZGVs YWxsb2Mgc3RhdGUgdHJhY2tpbmcgYnV0IG5vdAo+IAkgIGRhdGEtZXh0ZW50LWFscmVhZHkgZXhp c3RzIGxvb2t1cHMgZHVyaW5nIGEgc2Vlay4KPgo+IAktIGFzc3VtZXMgdGhhdCBhbGxvY2F0aW9u IHNpemUgaXMgdGhlIGJsb2NrIHNpemUgYW5kIHRoYXQKPiAJICBob2xlcyBmb2xsb3dzIGJsb2Nr IHNpemUgYWxpZ25tZW50LiBXZSBhbHJlYWR5IGtub3cgdGhhdAo+IAkgIGV4dDQgZG9lcyBub3Qg Zm9sbG93IHRoYXQgcnVsZSB3aGVuIGRvaW5nIHNtYWxsIHNwYXJzZQo+IAkgIHdyaXRlcyBjbG9z ZSB0b2dldGhlciBpbiBhIGZpbGUsIGFuZCBYRlMgaXMgYWxzbyBrbm93biB0bwo+IAkgIGZpbGwg aG9sZXMgd2hlbiBkb2luZyBzcGFyc2Ugd3JpdGVzIHBhc3QgRU9GLgo+Cj4gCS0gb25seSB0ZXN0 cyBzaW5nbGUgYmxvY2sgZGF0YSBleHRlbnRzINGVbyBkb2Vzbid0IGNvdmVyCj4gCSAgY29ybmVy IGNhc2VzIGxpa2Ugc2tpcHBpbmcgb3ZlciBtdWx0aXBsZSBmcmFnbWVudGVkIGRhdGEKPiAJICBl eHRlbnRzIHRvIHRoZSBuZXh0IGhvbGUuCj4KClllYWggSSBpbnRlbnRpb25hbGx5IGxlZnQgb3V0 IHByZWFsbG9jYXRlZCBzdHVmZiBiZWNhdXNlIHRoZXNlIGFyZSBnb2luZyAKdG8gYmUgaW1wbGVt ZW50YXRpb24gc3BlY2lmaWMsIHNvIEkgd2FzIGdvaW5nIHRvIGxlYXZlIHRoYXQgZm9yIGEgbGF0 ZXIgCmV4ZXJjaXNlIHdoZW4gcGVvcGxlIGFjdHVhbGx5IHN0YXJ0IGRvaW5nIHByb3BlciBpbXBs ZW1lbnRhdGlvbnMuCgo+IFNvbWUgbW9yZSBjb21tZW50cyBpbiBsaW5lLi4uLgo+Cj4+ICtfY2xl YW51cCgpCj4+ICt7Cj4+ICsgICAgcm0gLWYgJHRtcC4qCj4+ICt9Cj4+ICsKPj4gK3RyYXAgIl9j bGVhbnVwIDsgZXhpdCBcJHN0YXR1cyIgMCAxIDIgMyAxNQo+PiArCj4+ICsjIGdldCBzdGFuZGFy ZCBlbnZpcm9ubWVudCwgZmlsdGVycyBhbmQgY2hlY2tzCj4+ICsuIC4vY29tbW9uLnJjCj4+ICsu IC4vY29tbW9uLmZpbHRlcgo+PiArCj4+ICsjIHJlYWwgUUEgdGVzdCBzdGFydHMgaGVyZQo+PiAr X3N1cHBvcnRlZF9mcyBnZW5lcmljCj4+ICtfc3VwcG9ydGVkX29zIExpbnV4Cj4+ICsKPj4gK3Rl c3RmaWxlPSRURVNUX0RJUi9zZWVrX3Rlc3QuJCQKPj4gK2xvZ2ZpbGU9JFRFU1RfRElSL3NlZWtf dGVzdC4kJC5sb2cKPgo+IFRoZSBsb2cgZmlsZSBpcyB1c3VhbGx5IG5hbWVkICRzZXEuZnVsbCwg YW5kIGRvZXNuJ3QgZ2V0IHBsYWNlZCBpbgo+IHRoZSBmaWxlc3lzdGVtIGJlaW5nIHRlc3RlZC4g SXQgZ2V0cyBzYXZlZCBpbiB0aGUgeGZzdGVzdHMgZGlyZWN0b3J5Cj4gYWxvbmcgc2lkZSAkc2Vx Lm91dC5iYWQgZm9yIGFuYWx5c2lzIHdoZW50ZWggdGVzdCBmYWlscy4uLgo+CgpJIG9ubHkgd2Fu dCBpdCB0byBzZWUgaWYgU0VFS19IT0xFIGZhaWxzIHNvIEkgY2FuIHNheSBpdCBkaWRuJ3QgcnVu LiAgSSAKZm9sbG93ZWQgdGhlIHNhbWUgZXhhbXBsZSBhcyB0aGUgZmllbWFwIHRlc3QgdGhhdCBF cmljIHdyb3RlLgoKPj4gK1sgLXggJGhlcmUvc3JjL3NlZWstdGVzdGVyIF0gfHwgX25vdHJ1biAi c2Vlay10ZXN0ZXIgbm90IGJ1aWx0Igo+PiArCj4+ICtfY2xlYW51cCgpCj4+ICt7Cj4+ICsJcm0g LWYgJHRlc3RmaWxlCj4+ICsJcm0gLWYgJGxvZ2ZpbGUKPj4gK30KPj4gK3RyYXAgIl9jbGVhbnVw OyBleGl0IFwkc3RhdHVzIiAwIDEgMiAzIDE1Cj4+ICsKPj4gK2VjaG8gIlNpbGVuY2UgaXMgZ29s ZGVuIgo+PiArJGhlcmUvc3JjL3NlZWstdGVzdGVyIC1xICR0ZXN0ZmlsZSAyPiYxIHwgdGVlIC1h ICRsb2dmaWxlCj4KPiBQZXJzb25hbGx5IEknZCBwcmVmZXIgdGhlIHRlc3QgdG8gYmUgYSBiaXQg bm9pc3kgYWJvdXQgd2hhdCBpdCBpcwo+IHJ1bm5pbmcsIGVzcGVjaWFsbHkgd2hlbiB0aGVyZSBh cmUgc28gbWFueSBzdWJ0ZXN0cyB0aGUgc2luZ2xlCj4gaW52b2NhdGlvbiBpcyBydW5uaW5nLiBJ dCBtYWtlcyBubyBkaWZmZXJlbmNlIHRvIHRoZSBydW4gdGltZSBvZnRoZQo+IHRlc3QsIG9yIHRo ZSBvdXRwdXQgd2hlbiBzb21ldGhpbmcgZmFpbHMsIGJ1dCBpdCBhdCBsZWFzdCBhbGxvd3MgeW91 Cj4gdG8gcnVuIHRoZSB0ZXN0IG1hbnVhbGx5IGFuZCBzZWUgd2hhdCBpdCBpcyBkb2luZyBlYXNp bHkuLi4KPgoKUmlnaHQsIHRoZSBwcm9ibGVtIHdpdGggdGhpcyB0ZXN0IGlzIGl0IHdpbGwgcnVu IGRpZmZlcmVudGx5IGRlcGVuZGluZyAKb24gdGhlIGltcGxlbWVudGF0aW9uLiAgSSBhZ3JlZSwg SSByZWFsbHkgbGlrZSB0aGUgbm9pc3kgb3V0cHV0IHRlc3RzLCAKYnV0IHVuZm9ydHVuYXRlbHkg aWYgSSBydW4gdGhpcyB0ZXN0IG9uIGV4dDQgd2hlcmUgaXQgY3VycmVudGx5IHRyZWF0cyAKdGhl IGVudGlyZSBmaWxlIGFzIGRhdGEsIGFuZCB0aGVuIHJ1biBpdCBvbiBidHJmcyB3aGVyZSBpdCBp cyBzbWFydGVyIAphbmQgYWN0dWFsbHkgcmVjb2duaXplcyBob2xlcywgd2UgZW5kIHVwIHdpdGgg dHdvIGRpZmZlcmVudCBvdXRwdXRzIHRoYXQgCmFyZSBib3RoIGNvcnJlY3QuCgo+PiArCj4+ICtp ZiBncmVwIC1xICJTRUVLX0hPTEUgaXMgbm90IHN1cHBvcnRlZCIgJGxvZ2ZpbGU7IHRoZW4KPj4g Kwlfbm90cnVuICJTRUVLX0hPTEUvU0VFS19EQVRBIG5vdCBzdXBwb3J0ZWQgYnkgdGhpcyBrZXJu ZWwiCj4+ICtmaQo+PiArCj4+ICtybSAtZiAkbG9nZmlsZQo+PiArcm0gLWYgJHRlc3RmaWxlCj4+ ICsKPj4gK3N0YXR1cz0wIDsgZXhpdAo+PiBkaWZmIC0tZ2l0IGEvMjU1Lm91dCBiLzI1NS5vdXQK Pj4gbmV3IGZpbGUgbW9kZSAxMDA2NDQKPj4gaW5kZXggMDAwMDAwMC4uN2VlZmI4Mgo+PiAtLS0g L2Rldi9udWxsCj4+ICsrKyBiLzI1NS5vdXQKPj4gQEAgLTAsMCArMSwyIEBACj4+ICtRQSBvdXRw dXQgY3JlYXRlZCBieSAyNTUKPj4gK1NpbGVuY2UgaXMgZ29sZGVuCj4+IGRpZmYgLS1naXQgYS9n cm91cCBiL2dyb3VwCj4+IGluZGV4IDFmODYwNzUuLmMwNDVlNzAgMTAwNjQ0Cj4+IC0tLSBhL2dy b3VwCj4+ICsrKyBiL2dyb3VwCj4+IEBAIC0zNjgsMyArMzY4LDQgQEAgZGVwcmVjYXRlZAo+PiAg IDI1MiBhdXRvIHF1aWNrIHByZWFsbG9jCj4+ICAgMjUzIGF1dG8gcXVpY2sKPj4gICAyNTQgYXV0 byBxdWljawo+PiArMjU1IGF1dG8gcXVpY2sKPgo+IEknZCBzdWdnZXN0IHRoYXQgcncgYW5kIHBy ZWFsbG9jIChvbmNlIHVud3JpdHRlbiBleHRlbnQKPiB0ZXN0aW5nIGlzIGFkZGVkKSBncm91cHMg c2hvdWxkIGFsc28gYmUgZGVmaW5lZCBmb3IgdGhpcyB0ZXN0Lgo+Cj4gT3RoZXJ3aXNlLCB0aGUg dGVzdCBjb2RlIGxvb2tzIG9rIGlmIGEgYml0IG92ZXItZW5naW5lZXJlZC4uLi4KPgo+PiArc3Ry dWN0IHRlc3RyZWMgewo+PiArCWludAl0ZXN0X251bTsKPj4gKwlpbnQJKCp0ZXN0X2Z1bmMpKGlu dCBmZCwgaW50IHRlc3RudW0pOwo+PiArCWNoYXIJKnRlc3RfZGVzYzsKPj4gK307Cj4+ICsKPj4g K3N0cnVjdCB0ZXN0cmVjIHNlZWtfdGVzdHNbXSA9IHsKPj4gKwl7ICAxLCB0ZXN0MDEsICJUZXN0 IGJhc2ljIHN1cHBvcnQiIH0sCj4+ICsJeyAgMiwgdGVzdDAyLCAiVGVzdCBhbiBlbXB0eSBmaWxl IiB9LAo+PiArCXsgIDMsIHRlc3QwMywgIlRlc3QgYSBmdWxsIGZpbGUiIH0sCj4+ICsJeyAgNCwg dGVzdDA0LCAiVGVzdCBmaWxlIGhvbGUgYXQgYmVnLCBkYXRhIGF0IGVuZCIgfSwKPj4gKwl7ICA1 LCB0ZXN0MDUsICJUZXN0IGZpbGUgZGF0YSBhdCBiZWcsIGhvbGUgYXQgZW5kIiB9LAo+PiArCXsg IDYsIHRlc3QwNiwgIlRlc3QgZmlsZSBob2xlIGRhdGEgaG9sZSBkYXRhIiB9LAo+Cj4gU28sIHRv IHRha2UgZnJvbSB0aGUgaG9sZSBwdW5jaCB0ZXN0IG1hdHJpeCwgaXQgY292ZXJzIGEgYnVuY2gg bW9yZQo+IGZpbGUgc3RhdGUgdHJhbnNpdGlvbnMgYW5kIGNhc2VzIHRoYXQgYXJlIGp1c3QgYXMg cmVsZXZhbnQgdG8KPiBTRUVLX0hPTEUvU0VFS19EQVRBLiBUaG9zZSBjYXNlcyBhcmU6Cj4KPiAj ICAgICAgIDEuIGludG8gYSBob2xlCj4gIyAgICAgICAyLiBpbnRvIGFsbG9jYXRlZCBzcGFjZQo+ ICMgICAgICAgMy4gaW50byB1bndyaXR0ZW4gc3BhY2UKPiAjICAgICAgIDQuIGhvbGUgLT4gIGRh dGEKPiAjICAgICAgIDUuIGhvbGUgLT4gIHVud3JpdHRlbgo+ICMgICAgICAgNi4gZGF0YSAtPiAg aG9sZQo+ICMgICAgICAgNy4gZGF0YSAtPiAgdW53cml0dGVuCj4gIyAgICAgICA4LiB1bndyaXR0 ZW4gLT4gIGhvbGUKPiAjICAgICAgIDkuIHVud3JpdHRlbiAtPiAgZGF0YQo+ICMgICAgICAgMTAu IGhvbGUgLT4gIGRhdGEgLT4gIGhvbGUKPiAjICAgICAgIDExLiBkYXRhIC0+ICBob2xlIC0+ICBk YXRhCj4gIyAgICAgICAxMi4gdW53cml0dGVuIC0+ICBkYXRhIC0+ICB1bndyaXR0ZW4KPiAjICAg ICAgIDEzLiBkYXRhIC0+ICB1bndyaXR0ZW4gLT4gIGRhdGEKPiAjICAgICAgIDE0LiBkYXRhIC0+ ICBob2xlIEAgRU9GCj4gIyAgICAgICAxNS4gZGF0YSAtPiAgaG9sZSBAIDAKPiAjICAgICAgIDE2 LiBkYXRhIC0+ICBjYWNoZSBjb2xkIC0+aG9sZQo+ICMgICAgICAgMTcuIGRhdGEgLT4gIGhvbGUg aW4gc2luZ2xlIGJsb2NrIGZpbGUKPgo+IEkgdGhpa24gd2UgYWxzbyBuZWVkIHRvIGNvdmVyIG1v c3Qgb2YgdGhlc2Ugc2FtZSBjYXNlcywgcmlnaHQ/ICBBbmQKPiBTRUVLX0hPTEUvU0VFSyBkYXRh IGFsc28gbmVlZCB0byBleHBsaWNpdGx5IHNlcGFyYXRlIHRoZSB1bndyaXR0ZW4KPiB0ZXN0cyBp bnRvICJjbGVhbiB1bndyaXR0ZW4iIGFuZCAiZGlydHkgdW53cml0dGVuIiBhbmQgY292ZXIgdGhl Cj4gdHJhbnNpdGlvbnMgYmV0d2VlbiByZWdpb25zIG9mIHRob3NlIHN0YXRlcyBhcyB3ZWxsLCBy aWdodD8KPgoKWWVhaCB5b3UgYXJlIHJpZ2h0LCBidXQgYWdhaW4gZG9pbmcgcHJlYWxsb2NhdGVk IHN0dWZmIGlzIHRyaWNreSwgYnV0IEkgCmNhbiBleHBhbmQgaXQgbm93IGlmIHRoYXQncyB3aGF0 IHdlIHdhbnQuICBUaGFua3MsCgpKb3NlZgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18KeGZzIG1haWxpbmcgbGlzdAp4ZnNAb3NzLnNnaS5jb20KaHR0cDov L29zcy5zZ2kuY29tL21haWxtYW4vbGlzdGluZm8veGZzCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753021Ab1F2NTz (ORCPT ); Wed, 29 Jun 2011 09:19:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41927 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751318Ab1F2NTx (ORCPT ); Wed, 29 Jun 2011 09:19:53 -0400 Message-ID: <4E0B266D.30000@redhat.com> Date: Wed, 29 Jun 2011 09:19:41 -0400 From: Josef Bacik User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc15 Thunderbird/3.1.10 MIME-Version: 1.0 To: Dave Chinner CC: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org, xfs@oss.sgi.com, viro@ZenIV.linux.org.uk Subject: Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester References: <1309275199-10801-1-git-send-email-josef@redhat.com> <1309275199-10801-5-git-send-email-josef@redhat.com> <20110629065306.GC1026@dastard> In-Reply-To: <20110629065306.GC1026@dastard> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/29/2011 02:53 AM, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> This is a test to make sure seek_data/seek_hole is acting like it does on >> Solaris. It will check to see if the fs supports finding a hole or not and will >> adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. > > - cold cache behaviour - all dirty data ranges the test > creates are hot in cache and not even forced to disk, so > it is not testing the no-page-cache-over-the-data-range > case. i.e. it tests delalloc state tracking but not > data-extent-already exists lookups during a seek. > > - assumes that allocation size is the block size and that > holes follows block size alignment. We already know that > ext4 does not follow that rule when doing small sparse > writes close together in a file, and XFS is also known to > fill holes when doing sparse writes past EOF. > > - only tests single block data extents ѕo doesn't cover > corner cases like skipping over multiple fragmented data > extents to the next hole. > Yeah I intentionally left out preallocated stuff because these are going to be implementation specific, so I was going to leave that for a later exercise when people actually start doing proper implementations. > Some more comments in line.... > >> +_cleanup() >> +{ >> + rm -f $tmp.* >> +} >> + >> +trap "_cleanup ; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +# real QA test starts here >> +_supported_fs generic >> +_supported_os Linux >> + >> +testfile=$TEST_DIR/seek_test.$$ >> +logfile=$TEST_DIR/seek_test.$$.log > > The log file is usually named $seq.full, and doesn't get placed in > the filesystem being tested. It gets saved in the xfstests directory > along side $seq.out.bad for analysis whenteh test fails... > I only want it to see if SEEK_HOLE fails so I can say it didn't run. I followed the same example as the fiemap test that Eric wrote. >> +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" >> + >> +_cleanup() >> +{ >> + rm -f $testfile >> + rm -f $logfile >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +echo "Silence is golden" >> +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile > > Personally I'd prefer the test to be a bit noisy about what it is > running, especially when there are so many subtests the single > invocation is running. It makes no difference to the run time ofthe > test, or the output when something fails, but it at least allows you > to run the test manually and see what it is doing easily... > Right, the problem with this test is it will run differently depending on the implementation. I agree, I really like the noisy output tests, but unfortunately if I run this test on ext4 where it currently treats the entire file as data, and then run it on btrfs where it is smarter and actually recognizes holes, we end up with two different outputs that are both correct. >> + >> +if grep -q "SEEK_HOLE is not supported" $logfile; then >> + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" >> +fi >> + >> +rm -f $logfile >> +rm -f $testfile >> + >> +status=0 ; exit >> diff --git a/255.out b/255.out >> new file mode 100644 >> index 0000000..7eefb82 >> --- /dev/null >> +++ b/255.out >> @@ -0,0 +1,2 @@ >> +QA output created by 255 >> +Silence is golden >> diff --git a/group b/group >> index 1f86075..c045e70 100644 >> --- a/group >> +++ b/group >> @@ -368,3 +368,4 @@ deprecated >> 252 auto quick prealloc >> 253 auto quick >> 254 auto quick >> +255 auto quick > > I'd suggest that rw and prealloc (once unwritten extent > testing is added) groups should also be defined for this test. > > Otherwise, the test code looks ok if a bit over-engineered.... > >> +struct testrec { >> + int test_num; >> + int (*test_func)(int fd, int testnum); >> + char *test_desc; >> +}; >> + >> +struct testrec seek_tests[] = { >> + { 1, test01, "Test basic support" }, >> + { 2, test02, "Test an empty file" }, >> + { 3, test03, "Test a full file" }, >> + { 4, test04, "Test file hole at beg, data at end" }, >> + { 5, test05, "Test file data at beg, hole at end" }, >> + { 6, test06, "Test file hole data hole data" }, > > So, to take from the hole punch test matrix, it covers a bunch more > file state transitions and cases that are just as relevant to > SEEK_HOLE/SEEK_DATA. Those cases are: > > # 1. into a hole > # 2. into allocated space > # 3. into unwritten space > # 4. hole -> data > # 5. hole -> unwritten > # 6. data -> hole > # 7. data -> unwritten > # 8. unwritten -> hole > # 9. unwritten -> data > # 10. hole -> data -> hole > # 11. data -> hole -> data > # 12. unwritten -> data -> unwritten > # 13. data -> unwritten -> data > # 14. data -> hole @ EOF > # 15. data -> hole @ 0 > # 16. data -> cache cold ->hole > # 17. data -> hole in single block file > > I thikn we also need to cover most of these same cases, right? And > SEEK_HOLE/SEEK data also need to explicitly separate the unwritten > tests into "clean unwritten" and "dirty unwritten" and cover the > transitions between regions of those states as well, right? > Yeah you are right, but again doing preallocated stuff is tricky, but I can expand it now if that's what we want. Thanks, Josef