On Thu, Aug 25, 2016 at 10:37:09AM -0400, Brian Foster wrote: > On just skimming over this so far, I feel like this should be at least > two patches, possibly 3: > > - Kill xfs_bmapi_delay() and pull up associated bits to iomap(). As in just a move of code to xfs_iomap.c or also merged it with a partіal copy of xfs_file_iomap_begin? The first is trivial, but also rather pointless. The second is a bit more work, still very doable but probably also not that useful as we're going to totally rewrite it again in the next step. > - Possibly separate out the part that moves iteration from the (former) > xfs_bmapi_delay() code up to the iomap code, if we can do so cleanly. Well, the major point is that we get rid of the iteration as there isn't any actual need for it. > - Refactor/rework the preallocate logic. But I guess I could do a pass that creates xfs_file_iomap_begin_delay as in the new version except without the prealloc changes, and then separate them out. I don't quite see the point, though.. > I'm not necessarily against cleaning up/reworking the prealloc bits, but > I'm not a huge fan of open coding all of this here in the iomap > function. If nothing else, the indentation starts to make my eyes > cross... could we retain one level of abstraction here for this hunk of > logic that updates end_fsb? We're only having three tabs of indentation. I actually looked into a helper for that whole block, but we'd need to pass: ip, idx, prev, offset_fsb, offset, count, maxbytes_fsb (we could potentially re-derive offset_fsb from offset if we don't mind the inefficieny and recalculate maxbytes_fsb. This already assumes mp is trivially derived from ip) and return alloc_blocks, end_fsb so the function would be quite a monster in terms of its calling convention. Additionally we'd have the related by not qute the same if blocks around XFS_MOUNT_DFLT_IOSIZE and the isize split over two functions, which doesn't exactly help understanding the flow. _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
