2.6: drivers/md/dm-io.c partially copies bio.c

2.6: drivers/md/dm-io.c partially copies bio.c

[Adrian Bunk]

Hi,

drivers/md/dm-io.c copies functionality from bio.c .

Is there a specific reason why you don't simply use the functionality 
bio.c provides?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: 2.6: drivers/md/dm-io.c partially copies bio.c

[Kevin Corry]

Hi Adrian,

On Monday 06 December 2004 6:09 am, Adrian Bunk wrote:
> drivers/md/dm-io.c copies functionality from bio.c .
>
> Is there a specific reason why you don't simply use the functionality
> bio.c provides?

Can you give some specific examples of the functionality you think is 
duplicated? Meanwhile, I'll take a look and see if I can explain any code 
overlaps.

-- 
Kevin Corry
kevcorry@us.ibm.com
http://evms.sourceforge.net/

Re: 2.6: drivers/md/dm-io.c partially copies bio.c

[Jens Axboe]

On Mon, Dec 06 2004, Kevin Corry wrote:
> Hi Adrian,
> 
> On Monday 06 December 2004 6:09 am, Adrian Bunk wrote:
> > drivers/md/dm-io.c copies functionality from bio.c .
> >
> > Is there a specific reason why you don't simply use the functionality
> > bio.c provides?
> 
> Can you give some specific examples of the functionality you think is 
> duplicated? Meanwhile, I'll take a look and see if I can explain any code 
> overlaps.

Ah come on Kevin, a 2 second glance shows lots of uneccesary
duplication. Basically only the concept of the bio_set is not duplicated
in the first many lines, you even set up matching slabs.

How was that ever accepted for merging?

-- 
Jens Axboe

Re: 2.6: drivers/md/dm-io.c partially copies bio.c

[Kevin Corry]

On Monday 06 December 2004 7:55 am, Jens Axboe wrote:
> On Mon, Dec 06 2004, Kevin Corry wrote:
> > Hi Adrian,
> >
> > On Monday 06 December 2004 6:09 am, Adrian Bunk wrote:
> > > drivers/md/dm-io.c copies functionality from bio.c .
> > >
> > > Is there a specific reason why you don't simply use the functionality
> > > bio.c provides?
> >
> > Can you give some specific examples of the functionality you think is
> > duplicated? Meanwhile, I'll take a look and see if I can explain any code
> > overlaps.
>
> Ah come on Kevin, a 2 second glance shows lots of uneccesary
> duplication. Basically only the concept of the bio_set is not duplicated
> in the first many lines, you even set up matching slabs.
>
> How was that ever accepted for merging?

If I recall correctly (and it's been a while since I've looked at that code), 
the bio_set was added because a few DM modules like to initiate their own I/O 
requests (things like snapshots and DM's kcopyd daemon), and we felt it was 
better to allow these modules to each create their own mempools to allocate 
bios from, rather than allocate from the single kernel-wide bio pool used by 
the filesystem layer.

Strictly speaking (and as you mentioned), the slabs in the bio_set are 
unnecessary, and they could use the global bio_slab. But there's probably 
some argument to be made for having separate bio mempools for these modules.

Actually, I also seem to recall discussions with Joe Thornber from quite 
awhile ago about trying to move this bio_set functionality into fs/bio.c, to 
allow other device drivers to create their own private bio pools. If you 
think something like this would be desireable, I can try to look into the 
specifics again. If you think that having the single kernel-wide bio pool is 
sufficient for all filesystems and device-drivers (you certainly understand 
the trade-offs better than I do), then I can look into removing the necessary 
code from dm-io.c

-- 
Kevin Corry
kevcorry@us.ibm.com
http://evms.sourceforge.net/

Re: 2.6: drivers/md/dm-io.c partially copies bio.c

[Jens Axboe]

On Mon, Dec 06 2004, Kevin Corry wrote:
> On Monday 06 December 2004 7:55 am, Jens Axboe wrote:
> > On Mon, Dec 06 2004, Kevin Corry wrote:
> > > Hi Adrian,
> > >
> > > On Monday 06 December 2004 6:09 am, Adrian Bunk wrote:
> > > > drivers/md/dm-io.c copies functionality from bio.c .
> > > >
> > > > Is there a specific reason why you don't simply use the functionality
> > > > bio.c provides?
> > >
> > > Can you give some specific examples of the functionality you think is
> > > duplicated? Meanwhile, I'll take a look and see if I can explain any code
> > > overlaps.
> >
> > Ah come on Kevin, a 2 second glance shows lots of uneccesary
> > duplication. Basically only the concept of the bio_set is not duplicated
> > in the first many lines, you even set up matching slabs.
> >
> > How was that ever accepted for merging?
> 
> If I recall correctly (and it's been a while since I've looked at that
> code), the bio_set was added because a few DM modules like to initiate
> their own I/O requests (things like snapshots and DM's kcopyd daemon),
> and we felt it was better to allow these modules to each create their
> own mempools to allocate bios from, rather than allocate from the
> single kernel-wide bio pool used by the filesystem layer.

That is fully correct and also something that eg the bouncing code needs
to be 100% deadlock free.

> Strictly speaking (and as you mentioned), the slabs in the bio_set are
> unnecessary, and they could use the global bio_slab. But there's
> probably some argument to be made for having separate bio mempools for
> these modules.

The mempools are needed, the duplicated slabs are not. But that's just a
small part of it, basically the whole allocation and index stuff is 100%
duplicated from bio.c. Even bio_init().

> Actually, I also seem to recall discussions with Joe Thornber from
> quite awhile ago about trying to move this bio_set functionality into
> fs/bio.c, to allow other device drivers to create their own private
> bio pools. If you think something like this would be desireable, I can
> try to look into the specifics again. If you think that having the
> single kernel-wide bio pool is sufficient for all filesystems and
> device-drivers (you certainly understand the trade-offs better than I
> do), then I can look into removing the necessary code from dm-io.c

An abstraction for this would be nice, as there are two users of it then
(dm-io and highmem.c). So if your team would do that, I would sure help
you review and integrate it.

That's not what I'm arguing against, it's the (almost) wholesale
duplication that's a bit silly.

-- 
Jens Axboe

Re: 2.6: drivers/md/dm-io.c partially copies bio.c

[Kevin Corry]

On Monday 06 December 2004 8:42 am, Jens Axboe wrote:
> On Mon, Dec 06 2004, Kevin Corry wrote:
> > If I recall correctly (and it's been a while since I've looked at that
> > code), the bio_set was added because a few DM modules like to initiate
> > their own I/O requests (things like snapshots and DM's kcopyd daemon),
> > and we felt it was better to allow these modules to each create their
> > own mempools to allocate bios from, rather than allocate from the
> > single kernel-wide bio pool used by the filesystem layer.
>
> That is fully correct and also something that eg the bouncing code needs
> to be 100% deadlock free.
>
> > Strictly speaking (and as you mentioned), the slabs in the bio_set are
> > unnecessary, and they could use the global bio_slab. But there's
> > probably some argument to be made for having separate bio mempools for
> > these modules.
>
> The mempools are needed, the duplicated slabs are not. But that's just a
> small part of it, basically the whole allocation and index stuff is 100%
> duplicated from bio.c. Even bio_init().
>
> > Actually, I also seem to recall discussions with Joe Thornber from
> > quite awhile ago about trying to move this bio_set functionality into
> > fs/bio.c, to allow other device drivers to create their own private
> > bio pools. If you think something like this would be desireable, I can
> > try to look into the specifics again. If you think that having the
> > single kernel-wide bio pool is sufficient for all filesystems and
> > device-drivers (you certainly understand the trade-offs better than I
> > do), then I can look into removing the necessary code from dm-io.c
>
> An abstraction for this would be nice, as there are two users of it then
> (dm-io and highmem.c). So if your team would do that, I would sure help
> you review and integrate it.
>
> That's not what I'm arguing against, it's the (almost) wholesale
> duplication that's a bit silly.

I'll be happy to look into whether this duplication can be removed, and also 
whether the bio_set stuff could be moved out to bio.c or somewhere else 
common to the other device-drivers and filesystems. However, I've got a lot 
of stuff going on right now, so I won't really have time to get to this until 
after the start of the year (might be able to start looking into it over the 
holidays if I get everything else done in the next couple weeks). If/When I 
come up with some patches, I'll let you know.

-- 
Kevin Corry
kevcorry@us.ibm.com
http://evms.sourceforge.net/

Re: [dm-devel] Re: 2.6: drivers/md/dm-io.c partially copies bio.c

[Alasdair G Kergon]

On Mon, Dec 06, 2004 at 08:22:18AM -0600, Kevin Corry wrote:
> Actually, I also seem to recall discussions with Joe Thornber from quite 
> awhile ago about trying to move this bio_set functionality into fs/bio.c, 

Indeed - see his comments in dm-io.c:

/*-----------------------------------------------------------------
 * Bio set, move this to bio.c
 *---------------------------------------------------------------*/

...

/*-----------------------------------------------------------------
 * dm-io proper
 *---------------------------------------------------------------*/


Alasdair
-- 
agk@redhat.com