[RFC,00/11] iomap: regroup code by functional area
mbox series

Message ID 156200051933.1790352.5147420943973755350.stgit@magnolia
Headers show
Series
  • iomap: regroup code by functional area
Related show

Message

Darrick J. Wong July 1, 2019, 5:01 p.m. UTC
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.

--D

Comments

Theodore Y. Ts'o July 1, 2019, 5:41 p.m. UTC | #1
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
Darrick J. Wong July 1, 2019, 5:59 p.m. UTC | #2
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
Christoph Hellwig July 8, 2019, 6:46 p.m. UTC | #3
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.
Darrick J. Wong July 9, 2019, 4:49 p.m. UTC | #4
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
Christoph Hellwig July 9, 2019, 6:12 p.m. UTC | #5
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
Darrick J. Wong July 15, 2019, 4:43 p.m. UTC | #6
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
Christoph Hellwig July 15, 2019, 4:50 p.m. UTC | #7
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).
Darrick J. Wong July 15, 2019, 5:49 p.m. UTC | #8
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