On Friday, June 24, 2016 10:50:41 AM Qu Wenruo wrote: > Hi Chandan, David, > > When I'm trying to rebase dedupe patchset on top of Chadan's sub page > size patchset (using David's for-next-test-20160620), although the > rebase itself is quite simple, but I'm afraid that I found some bugs for > sub page size patchset, *without* dedupe patchset applied. > > These bugs seems to be unrelated to each other > 1) state leak at btrfs rmmod time The leak was due to not freeing 'cached_state' in read_extent_buffer_pages(). I have fixed this and the fix will be part of the patchset when I post the next version to the mailing list. I have always compiled the btrfs code as part of the vmlinux image and hence have never rmmod the btrfs module during my local testing. The space leak messages might have appeared when I shut down my guest. Hence I had never noticed them before. Thanks once again for informing me about it. > 2) bytes_may_use leak at qgroup EDQUOTA error time I have a slightly older version of btrfs-progs which does not yet have btrfs dedupe" command. I will get the new version and check if the space leak can be reproduced on my machine. However, I don't see the space leak warning messages when the reproducer script is executed after commenting out "btrfs dedupe enable $mnt". > 3) selftest is run several times at modules load time > 15 times, to be more exact > And since I didn't found any immediate number related to run it 15 > times, I assume at least it's not designed to do it 15 times. > > The reproducer for 1) and 2) is quite simple, extracted from btrfs/022 > test case: > ------ > dev=/dev/sdb5 > mnt=/mnt/test > > umount $dev &> /dev/null > > mkfs.btrfs $dev -f > mount $dev $mnt -o nospace_cache > btrfs dedupe enable $mnt > btrfs sub create $mnt/sub > btrfs quota enable $mnt > > > # Just use small limit, making ftrace less noise. > btrfs qgroup limit 512K 0/257 $mnt > dd if=/dev/urandom of=$mnt/sub/test bs=1M count=1 > umount $mnt > rmmod btrfs > ------ > > At unmount time, kernel warning will happen due to may_use bytes leak. > I could dig it further, as it looks like a bug in space reservation > failure case. > ------ > BTRFS: space_info 1 has 8044544 free, is not full > BTRFS: space_info total=8388608, used=344064, pinned=0, reserved=0, > may_use=409600, readonly=0 > ------ > > And at rmmod time, btrfs will detect extent_state leak, whose length is > always 4095 (page size - 1). > > Hope this will help, and I'm willing to help to fix the problem. > > Thanks, > Qu > > At 06/23/2016 08:17 PM, David Sterba wrote: > > On Tue, Jun 21, 2016 at 10:25:19PM +0530, Chandan Rajendra wrote: > >>>> I'm completely OK to do the rebase, but since I don't have 64K page size > >>>> machine to test the rebase, we can only test if 4K system is unaffected. > >>>> > >>>> Although not much help, at least it would be better than making it compile. > >>>> > >>>> Also such rebase may help us to expose bad design/unexpected corner case > >>>> in dedupe. > >>>> So if it's OK, please let me try to do the rebase. > >>> > >>> Well, if you base dedupe on subpage, then it could be hard to find the > >>> patchset that introduces bugs, or combination of both. We should be able > >>> to test the features independently, and thus I'm proposing to first find > >>> some common patchset that makes that possible. > >> > >> I am not sure if I understood the above statement correctly. Do you mean to > >> commit the 'common/simple' patches from both the subpage-blocksize & dedupe > >> patchset first and then bring in the complicated ones later? > > > > That would be great yes, but ... > > > >> If yes, then we have a problem doing that w.r.t subpage-blocksize > >> patchset. The first few patches bring in the core changes necessary for the > >> other remaining patches. > > > > ... not easily possible. I looked again for common functions that change > > the singature and found only cow_file_range and run_delalloc_nocow. The > > plan: > > > > - separate patch that adds new parameters required by both patches to > > the functions > > - update all call sites, add 0/NULL as defaults for the new unused > > parameters > > - rebase both patches on top of this patch > > > > How does this help: if a patch starts to use the new parameter, it > > changes only the value at all call sites. This is much easier to verify > > and merge manually compared to adding a new parameter to the middle of > > the list, namely when the functions take 6+. > > > > The other conflicts like conversion from PAGE_SIZE to the the block > > oriented iterations will be harder, but these are usually localized and > > can be resolved. We'll see if there are other options to reduce the > > clashes but at the moment it's stuck at the two functions. Does that > > explain it better? > > > > > > -- chandan -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
