| Message ID | 156200051933.1790352.5147420943973755350.stgit@magnolia (mailing list archive) |
|---|---|
| Headers | show |
| Series | iomap: regroup code by functional area | expand |
On Mon, Jul 01, 2019 at 10:01:59AM -0700, Darrick J. Wong wrote: > Note that this is not the final format of the patches, because I intend > to pick a point towards the end of the merge window (after everyone > else's merges have landed), rebase this series atop that, and push it > back to Linus. So normally Linus isn't psyched about pulling branches that were rebased at the last minute. I guess we could ask him ahead of time if he's OK with this plan. Or have you done that already? Alternatively you could rebase this on top of v5.3-rc2, after the merge window closes, and get agreement from the 4 file systems which are currently iomap users: ext2, ext4, gfs2, and xfs to start their development trees on top of that common branch for the 5.4 merge window. After all, it's just moving code around and there are no substantive changes in this patch series, right? So there's no rush as I understand things for this to hit mainline. Cheers, - Ted
On Mon, Jul 01, 2019 at 01:41:29PM -0400, Theodore Ts'o wrote: > On Mon, Jul 01, 2019 at 10:01:59AM -0700, Darrick J. Wong wrote: > > Note that this is not the final format of the patches, because I intend > > to pick a point towards the end of the merge window (after everyone > > else's merges have landed), rebase this series atop that, and push it > > back to Linus. > > So normally Linus isn't psyched about pulling branches that were > rebased at the last minute. I guess we could ask him ahead of time if > he's OK with this plan. Or have you done that already? I've not done so yet, since this is the first time this cleanup has been posted. Assuming I don't hear any loud complaining from anyone, I was going to send the existing iomap 5.3 patches (all three of them) to Linus at the start of the merge window and let him know that I'd like to do the quick cleanup during the second week. I've done tree cleanups this way before without hearing any complaints, though they've never gone outside the xfs tree. > Alternatively you could rebase this on top of v5.3-rc2, after the > merge window closes, and get agreement from the 4 file systems which > are currently iomap users: ext2, ext4, gfs2, and xfs to start their > development trees on top of that common branch for the 5.4 merge > window. After all, it's just moving code around and there are no > substantive changes in this patch series, right? So there's no rush > as I understand things for this to hit mainline. <shrug> I prefer to get this done quickly at the end of the 5.3 merge window to reduce the risk that I'll have to reconcile the cleanup with whatever iomap fixes land during -rc1 to -rc7. I don't know if you've been following the "lift xfs writeback to iomap" thread lately but I've also been thinking about putting out a work branch for 5.4 with the sample writeback code in fs/iomap/writeback.c to see if anyone bites, so we're likely to get such a tree anyway. --D > Cheers, > > - Ted
On Mon, Jul 01, 2019 at 10:01:59AM -0700, Darrick J. Wong wrote: > Hi all, > > This series breaks up fs/iomap.c by grouping the functions by major > functional area (swapfiles, fiemap, seek hole/data, directio, buffered > writes, buffered reads, page management, and page migration) in separate > source code files under fs/iomap/. No functional changes have been > made. > > Note that this is not the final format of the patches, because I intend > to pick a point towards the end of the merge window (after everyone > else's merges have landed), rebase this series atop that, and push it > back to Linus. The RFC is posted so that everyone can provide feedback > on the grouping strategy, not line-specific code movements. > > This has been lightly tested with fstests. Enjoy! > Comments and questions are, as always, welcome. Do you have a branch somewhere for the layout? To me it seems to be a little too fine grained and creates tons of tiny files, which make hacking the code painful.
On Mon, Jul 08, 2019 at 11:46:52AM -0700, Christoph Hellwig wrote: > On Mon, Jul 01, 2019 at 10:01:59AM -0700, Darrick J. Wong wrote: > > Hi all, > > > > This series breaks up fs/iomap.c by grouping the functions by major > > functional area (swapfiles, fiemap, seek hole/data, directio, buffered > > writes, buffered reads, page management, and page migration) in separate > > source code files under fs/iomap/. No functional changes have been > > made. > > > > Note that this is not the final format of the patches, because I intend > > to pick a point towards the end of the merge window (after everyone > > else's merges have landed), rebase this series atop that, and push it > > back to Linus. The RFC is posted so that everyone can provide feedback > > on the grouping strategy, not line-specific code movements. > > > > This has been lightly tested with fstests. Enjoy! > > Comments and questions are, as always, welcome. > > Do you have a branch somewhere for the layout? I sent it out to for-next to see what it would collide with: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/log/?h=iomap-5.3-merge Though I'll probably rebase it after the mm and block merges (which might have already happened). > To me it seems to be a little too fine grained and creates tons of tiny > files, which make hacking the code painful. It's nine files and now code is grouped by functional area which makes it easier to keep track of which functions go with the publicly exposed iomap apis. I don't think we're going to more than a half dozen more files ever. --D
I looked over it and while some of the small files seem very tiny they are reasonably split. What rather annoys me is the page.c/read.c/write.c split. All these really belong mostly together, except maybe the super highlevel write code that then either calls into the buffer_head vs iomap_page based code. By keeping them together we can eliminate most of iomap_internal.h and once the writeback code moves also keep iomap_page private to that bigger read.c file. A few other minor notes: - I think iomap_sector() should move to linux/iomap.h as an inline helper. - iomap_actor_t / iomap_apply should probaby just move to linux/iomap.h as well, which would avoid needing the awkward subdir include in dax.c - some of the copyrights for the small files seem totally wrong. e.g. all the swapfile code was written by you, so it should not have my or rh copyright notices on it
On Tue, Jul 09, 2019 at 11:12:14AM -0700, Christoph Hellwig wrote: > I looked over it and while some of the small files seem very tiny > they are reasonably split. > > What rather annoys me is the page.c/read.c/write.c split. All these > really belong mostly together, except maybe the super highlevel > write code that then either calls into the buffer_head vs iomap_page > based code. By keeping them together we can eliminate most of > iomap_internal.h and once the writeback code moves also keep > iomap_page private to that bigger read.c file. <nod> I think it makes sense to combine them into a single read_write.c file or something. > > A few other minor notes: > > - I think iomap_sector() should move to linux/iomap.h as an inline > helper. So long as you're ok with including blkdev.h from iomap.h. :) > - iomap_actor_t / iomap_apply should probaby just move to linux/iomap.h > as well, which would avoid needing the awkward subdir include in > dax.c I'd also like to fix the opencoded iomap_apply() in fs/dax.c, but cleaning all that up looks stressful enough to defer for 5.4. > - some of the copyrights for the small files seem totally wrong. > e.g. all the swapfile code was written by you, so it should not have > my or rh copyright notices on it Will fix the swapfile code. --D
On Mon, Jul 15, 2019 at 09:43:07AM -0700, Darrick J. Wong wrote: > On Tue, Jul 09, 2019 at 11:12:14AM -0700, Christoph Hellwig wrote: > > I looked over it and while some of the small files seem very tiny > > they are reasonably split. > > > > What rather annoys me is the page.c/read.c/write.c split. All these > > really belong mostly together, except maybe the super highlevel > > write code that then either calls into the buffer_head vs iomap_page > > based code. By keeping them together we can eliminate most of > > iomap_internal.h and once the writeback code moves also keep > > iomap_page private to that bigger read.c file. > > <nod> I think it makes sense to combine them into a single read_write.c > file or something. page.c or buffered-io.c seems like sensible names to me. > > - some of the copyrights for the small files seem totally wrong. > > e.g. all the swapfile code was written by you, so it should not have > > my or rh copyright notices on it > > Will fix the swapfile code. Please also look over the other files, a few of them should probably be just me (e.g. fiemap) and some have other authors (seek is mostly Andreas with a few later bits from me).
On Mon, Jul 15, 2019 at 09:50:13AM -0700, Christoph Hellwig wrote: > On Mon, Jul 15, 2019 at 09:43:07AM -0700, Darrick J. Wong wrote: > > On Tue, Jul 09, 2019 at 11:12:14AM -0700, Christoph Hellwig wrote: > > > I looked over it and while some of the small files seem very tiny > > > they are reasonably split. > > > > > > What rather annoys me is the page.c/read.c/write.c split. All these > > > really belong mostly together, except maybe the super highlevel > > > write code that then either calls into the buffer_head vs iomap_page > > > based code. By keeping them together we can eliminate most of > > > iomap_internal.h and once the writeback code moves also keep > > > iomap_page private to that bigger read.c file. > > > > <nod> I think it makes sense to combine them into a single read_write.c > > file or something. > > page.c or buffered-io.c seems like sensible names to me. buffered-io.c it is then. > > > - some of the copyrights for the small files seem totally wrong. > > > e.g. all the swapfile code was written by you, so it should not have > > > my or rh copyright notices on it > > > > Will fix the swapfile code. > > Please also look over the other files, a few of them should probably > be just me (e.g. fiemap) and some have other authors (seek is mostly > Andreas with a few later bits from me). <nod> I'll edit the copyrights on fiemap.c. I'm less sure about making edits to seek.c because I don't know if Andreas has copyright ownership or if RH slurped all that up -- the commits are from his @redhat.com email. I was about to resend the series so I'll cc him on it. --D