block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

On Wed, Aug 22, 2012 at 10:04:09AM -0700, Kent Overstreet wrote:
> Previously, there was bio_clone() but it only allocated from the fs bio
> set; as a result various users were open coding it and using
> __bio_clone().
>
> This changes bio_clone() to become bio_clone_bioset(), and then we add
> bio_clone() and bio_clone_kmalloc() as wrappers around it, making use of
> the functionality the last patch adedd.
>
> This will also help in a later patch changing how bio cloning works.

I'd prefer simply adding @bioset to bio_clone() so that the caller
always has to make the choice consciously. We're updating all the
callers anyway.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

On Wed, Aug 22, 2012 at 02:07:40PM -0700, Tejun Heo wrote:
> On Wed, Aug 22, 2012 at 10:04:09AM -0700, Kent Overstreet wrote:
> > Previously, there was bio_clone() but it only allocated from the fs bio
> > set; as a result various users were open coding it and using
> > __bio_clone().
> >
> > This changes bio_clone() to become bio_clone_bioset(), and then we add
> > bio_clone() and bio_clone_kmalloc() as wrappers around it, making use of
> > the functionality the last patch adedd.
> >
> > This will also help in a later patch changing how bio cloning works.
>
> I'd prefer simply adding @bioset to bio_clone() so that the caller
> always has to make the choice consciously. We're updating all the
> callers anyway.

Possibly, but the btrfs code uses bio_clone() and there fs_bio_set may
be correct (will have to look at what it's doing, if it's cloning a bio
that was allocated out of fs_bio_set that would be bad..)

I would also prefer to simply drop bio_clone() so that
bio_clone_bioset() matches bio_alloc_bioset(), but regardless that'll
have to be a different patch (and I don't think I've had to update any
of the bio_clone() callers in this patch series anyways).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

Hello,

On Thu, Aug 23, 2012 at 11:24:18PM -0700, Kent Overstreet wrote:
> > I'd prefer simply adding @bioset to bio_clone() so that the caller
> > always has to make the choice consciously. We're updating all the
> > callers anyway.
>
> Possibly, but the btrfs code uses bio_clone() and there fs_bio_set may
> be correct (will have to look at what it's doing, if it's cloning a bio
> that was allocated out of fs_bio_set that would be bad..)

Yeah, I think it's generally a good idea to require explicit bioset
specification even if that ends up being fs_bio_set or NULL.

> I would also prefer to simply drop bio_clone() so that
> bio_clone_bioset() matches bio_alloc_bioset(), but regardless that'll
> have to be a different patch (and I don't think I've had to update any
> of the bio_clone() callers in this patch series anyways).

Ooh yeah, bio_clone_bioset() would be the better name for it.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

On Tue, Aug 28, 2012 at 10:37:34AM -0700, Kent Overstreet wrote:
> +static inline struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
> +{
> + return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
> +}
> +
...
> +static inline struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask)
> +{
> + return bio_clone_bioset(bio, gfp_mask, NULL);
> +
> +}

Do we really need these wrappers? I'd prefer requiring users to
explicit choose @bioset when cloning.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

Hello,

On Tue, Aug 28, 2012 at 03:05:32PM -0700, Kent Overstreet wrote:
> On Tue, Aug 28, 2012 at 01:44:01PM -0700, Tejun Heo wrote:
> > On Tue, Aug 28, 2012 at 10:37:34AM -0700, Kent Overstreet wrote:
> > > +static inline struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
> > > +{
> > > + return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
> > > +}
> > > +
> > ...
> > > +static inline struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask)
> > > +{
> > > + return bio_clone_bioset(bio, gfp_mask, NULL);
> > > +
> > > +}
> >
> > Do we really need these wrappers? I'd prefer requiring users to
> > explicit choose @bioset when cloning.
>
> bio_clone() is an existing api, I agree but I'd prefer to convert
> existing users in a separate patch and when I do that I want to spend
> some time actually looking at the existing code instead of doing the
> conversion blindly (at least some of the existing users are incorrect
> and I'll have to add bio_sets for them).

Aren't there like three users in kernel? If you wanna clean it up
later, that's fine too but I don't think it would make much difference
either way.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

On Thu, Sep 06, 2012 at 03:35:02PM -0700, Kent Overstreet wrote:
> Previously, there was bio_clone() but it only allocated from the fs bio
> set; as a result various users were open coding it and using
> __bio_clone().
>
> This changes bio_clone() to become bio_clone_bioset(), and then we add
> bio_clone() and bio_clone_kmalloc() as wrappers around it, making use of
> the functionality the last patch adedd.
>
> This will also help in a later patch changing how bio cloning works.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: NeilBrown <neilb@suse.de>
> CC: Alasdair Kergon <agk@redhat.com>
> CC: Boaz Harrosh <bharrosh@panasas.com>
> CC: Jeff Garzik <jeff@garzik.org>
> Acked-by: Jeff Garzik <jgarzik@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

block: Add bio_clone_bioset(), bio_clone_kmalloc()
 

On Fri, Sep 14, 2012 at 10:50:59PM +0100, Alasdair G Kergon wrote:
> On Thu, Sep 06, 2012 at 03:35:02PM -0700, Kent Overstreet wrote:
> > Previously, there was bio_clone() but it only allocated from the fs bio
> > set; as a result various users were open coding it and using
> > __bio_clone().
>
> Explain in the header the reasoning behind the change to dm-crypt so that
> it no longer resets bi_idx to 0 too?
>
> > - clone->bi_idx = 0;

Previously, it was open coding __bio_clone(), that's what the setting
bi_idx to 0 was from. With the change to bio_clone_bioset() that's no
longer necessary (and dangerous, since bi_idx needs to be consistent
with bi_sector/bi_size).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel