| Message ID | 1464702291.900.75.camel@linux-q3cb.site (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Krzysztof, > I can check but here are my remarks: > the dip2vip_cpy() does partial byte-swap from file system endian to cpu > endian, so the union uses fs native endianess and this raises difficulty > of using struct vxfs_inode_info to yet another level. > Is this a good practice ? (apart from a benefit like some minor speed > gain) It seems easier this way around. If you think byte swapping makes life easier for you I'm happy to take an incremental patch. Howerver that means we'll need separate structure defintions for the union members. > Are there any real benefits from marking anything "bitwise" except that > it is just another type ? bitwise is an annotation for sparse so that you can't directly assign to the variable, and need to use accessors instead. In this case it's used so that we are forced to use the byte swapping helpers and get a warning from sparse if that's not done properly. > > because for some strange reason the patch uses "PAGE_SHIFT" while > original lookup.c from 3.16 or 3.18 kernels uses PAGE_CACHE_SHIFT but > 4.6 kernel. so the patch below does not apply cleanly to source from 3.x > kernels. I use 3.16, double checked latest 3.18. Yes, PAGE_CACHE_SIZE, PAGE_CACHE_SHIFT and co were removed in kernel 4.6, as they have always been identical to the versions without _CACHE. > Anyway because readdir() is not aware of fs endianess (because 0004 > patch is not in there) the hp-ux tests will fail with some general > protection fault very likely. I think all readdir structures are fixed up now, please check. > I can't see also patches which fix these obvious bugs like freeing > kmem_cache_alloc() objects with kfree. > Even worse is releasing inode->i_private from the "evict_inode" > callback. > This leads to dangling objects in vxfs's kmem_cache when the fs is > unmounted. Destroying cache on module unload causes kmem_cache_destroy > to complain about it and after a few tries ((module load, mount, some > ops on mountpoint, unmount, unload) x few times) hard lockup is > inevitable. Yeah, this was just getting started. I've spent some more time on the whole inode issues and have implemented this a bit different from what you did, although the end result is very similar. Can you take a look at the tree here: http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/freevxfs > Do you think other patches should be updated with regard to the patch > you sent ? Please take a look at the branch above. The only major thing that should be missing is your directory code refactoring. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Christoph, On Wed, 2016-06-01 at 00:33 -0700, Christoph Hellwig wrote: > Hi Krzysztof, > > > I can check but here are my remarks: > > the dip2vip_cpy() does partial byte-swap from file system endian to cpu > > endian, so the union uses fs native endianess and this raises difficulty > > of using struct vxfs_inode_info to yet another level. > > Is this a good practice ? (apart from a benefit like some minor speed > > gain) > > It seems easier this way around. If you think byte swapping makes > life easier for you I'm happy to take an incremental patch. Howerver > that means we'll need separate structure defintions for the union > members. yes, you are right. bitwise attribute is convenient for endian annotation for automated checks. I did not know this feature and it seemed like excessive typing overhead to me. Anyway I must admit that it has benefits. Without that I wouldn't dare to have a mixed endian structure. I would just put a //note somewhere aside for me to remember that this structure requires special handling with regard to endianess and whether it has been copied already or it is just mapped piece of memory from backend device. Would I do this with every member of the structure ? of course, wouldn't. (too many to remember) > > > Are there any real benefits from marking anything "bitwise" except that > > it is just another type ? > > bitwise is an annotation for sparse so that you can't directly assign > to the variable, and need to use accessors instead. In this case it's > used so that we are forced to use the byte swapping helpers and get a > warning from sparse if that's not done properly. > indeed, I realized this. > > > > because for some strange reason the patch uses "PAGE_SHIFT" while > > original lookup.c from 3.16 or 3.18 kernels uses PAGE_CACHE_SHIFT but > > 4.6 kernel. so the patch below does not apply cleanly to source from 3.x > > kernels. I use 3.16, double checked latest 3.18. > > Yes, PAGE_CACHE_SIZE, PAGE_CACHE_SHIFT and co were removed in kernel > 4.6, as they have always been identical to the versions without _CACHE. I see. sorry, I reworked these remaining patches with 3.16 so you will hit the issue with applying them again. I will post them shortly soon. Good news is that regression tests are ok. The patchset will start from 2nd patch. I think there is no need to re-post the patch you created. > > > Anyway because readdir() is not aware of fs endianess (because 0004 > > patch is not in there) the hp-ux tests will fail with some general > > protection fault very likely. > > I think all readdir structures are fixed up now, please check. right. > > > I can't see also patches which fix these obvious bugs like freeing > > kmem_cache_alloc() objects with kfree. > > Even worse is releasing inode->i_private from the "evict_inode" > > callback. > > This leads to dangling objects in vxfs's kmem_cache when the fs is > > unmounted. Destroying cache on module unload causes kmem_cache_destroy > > to complain about it and after a few tries ((module load, mount, some > > ops on mountpoint, unmount, unload) x few times) hard lockup is > > inevitable. > > Yeah, this was just getting started. I've spent some more time on the > whole inode issues and have implemented this a bit different from what > you did, although the end result is very similar. Can you take a look > at the tree here: > > http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/freevxfs > > > Do you think other patches should be updated with regard to the patch > > you sent ? > okay .. once again we did the same thing in parallel without a mutex ... will see your solution. we will need to sort out which one is better and/or looks better. I am afraid that I may loose because don't care about tabs&spaces. > Please take a look at the branch above. The only major thing that > should be missing is your directory code refactoring. Thanks. yes, the old readdir has a bug. this time my change logs are more verbose.
Hi Christoph, On Wed, 2016-06-01 at 00:33 -0700, Christoph Hellwig wrote: > Yeah, this was just getting started. I've spent some more time on the > whole inode issues and have implemented this a bit different from what > you did, although the end result is very similar. Can you take a look > at the tree here: > > http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/freevxfs > > > Do you think other patches should be updated with regard to the patch > > you sent ? > > Please take a look at the branch above. I think that there are no "kfree(pfp); kfree(sfp); return 0;" in vxfs_read_fshead() still. Are pfp and sfp needed anywhere ? I am sure they are not so there is a memory leak without these kfrees every mount. I am not sure absolutely of that read_fshead() is missing these kfrees because I have seen just these diffs, anyway I did not notice "+kfree". so .. whose patch is more accurate ? needles to say that I prefer to have limited scope of visibility of inode_cachep to the inode.c only. Thanks,
I forgot to say that http://git.infradead.org/users/hch/vfs.git/commit/1cce17017970c0797943e069cc520e17d068ca4b looks good. I am fine with it so let's forget about 4th "the credits" patch I have posted. Thanks !
On Wed, Jun 01, 2016 at 11:23:32AM +0200, Krzysztof B??aszkowski wrote: > I think that there are no "kfree(pfp); kfree(sfp); return 0;" in > vxfs_read_fshead() still. Are pfp and sfp needed anywhere ? I am sure > they are not so there is a memory leak without these kfrees every mount. > > I am not sure absolutely of that read_fshead() is missing these kfrees > because I have seen just these diffs, anyway I did not notice "+kfree". The frees are still missing. Do you want to send me a patch for those? > needles to say that I prefer to have limited scope of visibility of > inode_cachep to the inode.c only. In my tree the visibility is in vxfs_super.c only. Given that the alloc/destroy methods are super operations that seems to fit better as they can have local scope, too. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-06-02 at 01:25 -0700, Christoph Hellwig wrote: > On Wed, Jun 01, 2016 at 11:23:32AM +0200, Krzysztof B??aszkowski wrote: > > I think that there are no "kfree(pfp); kfree(sfp); return 0;" in > > vxfs_read_fshead() still. Are pfp and sfp needed anywhere ? I am sure > > they are not so there is a memory leak without these kfrees every mount. > > > > I am not sure absolutely of that read_fshead() is missing these kfrees > > because I have seen just these diffs, anyway I did not notice "+kfree". > > The frees are still missing. Do you want to send me a patch for those? I see. Don't hesitate to add them, I was thinking that creating a special patch because of these just two kfree()s is just too much formal work. > > > needles to say that I prefer to have limited scope of visibility of > > inode_cachep to the inode.c only. > > In my tree the visibility is in vxfs_super.c only. Given that the > alloc/destroy methods are super operations that seems to fit better > as they can have local scope, too. I see. We differ in point of views. I presumed that it is more consistent to have everything regarding inodes creation (getting)/destroying, etc in the inode.c. Anyway this is just pure academic debate, we could argue on anything else (weather, politics and politicians - hot topic especially in Poland) as well. You decide. You enjoy more your approach - good enough.
--- vxfs_lookup.c +++ vxfs_lookup.c @@ -298,8 +306,10 @@ offset = (char *)de - kaddr; ctx->pos = ((page << PAGE_SHIFT) | offset) + 2; - if (!dir_emit(ctx, de->d_name, de->d_namelen, - de->d_ino, DT_UNKNOWN)) { + if (!dir_emit(ctx, de->d_name, + fs16_to_cpu(sbi, de->d_namelen), + fs32_to_cpu(sbi, de->d_ino), + DT_UNKNOWN)) { vxfs_put_page(pp); return 0; }