From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boaz Harrosh Subject: Re: [PATCH v4 03/12] block: Add bio_reset() Date: Wed, 25 Jul 2012 14:19:27 +0300 Message-ID: <500FD63F.7050501@panasas.com> References: <1343160689-12378-1-git-send-email-koverstreet@google.com> <1343160689-12378-4-git-send-email-koverstreet@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1343160689-12378-4-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Sender: linux-bcache-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Kent Overstreet Cc: linux-bcache-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org, agk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, neilb-l3A5Bk7waGM@public.gmane.org, drbd-dev-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org, vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, mpatocka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, sage-BnTBU8nroG7k1uMJSBkQmQ@public.gmane.org, yehuda-L5o5AL9CYN0tUFlbccrkMA@public.gmane.org List-Id: dm-devel.ids On 07/24/2012 11:11 PM, Kent Overstreet wrote: > Reusing bios is something that's been highly frowned upon in the past, > but driver code keeps doing it anyways. If it's going to happen anyways, > we should provide a generic method. > > This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c > was open coding it, by doing a bio_init() and resetting bi_destructor. > > Signed-off-by: Kent Overstreet > --- > fs/bio.c | 10 ++++++++++ > include/linux/bio.h | 1 + > include/linux/blk_types.h | 6 ++++++ > 3 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/fs/bio.c b/fs/bio.c > index 1c6c8b7..252e253 100644 > --- a/fs/bio.c > +++ b/fs/bio.c > @@ -261,6 +261,16 @@ void bio_init(struct bio *bio) > } > EXPORT_SYMBOL(bio_init); > > +void bio_reset(struct bio *bio) > +{ > + /* Clear all flags below BIO_OWNS_VEC */ > + unsigned long flags = bio->bi_flags & (~0UL << BIO_OWNS_VEC); > + Hey I have not seen these FLAGS thing before. Are these new? Anyway. Please NO!!! for one you need to put a big fat comment over at flags definitions. And two what happens when one adds a new flag. Is it reset or not reset? I'd rather you define a flags mask for those that need to be preserved, at header, plus a comment that any needed-to-be-preserved cross init flags, must be added to the mask. Never, ever, hide such crap so deep in the code. Boaz > + memset(bio, 0, BIO_RESET_BYTES); > + bio->bi_flags = flags|(1 << BIO_UPTODATE); > +} > +EXPORT_SYMBOL(bio_reset); > + > /** > * bio_alloc_bioset - allocate a bio for I/O > * @gfp_mask: the GFP_ mask given to the slab allocator > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 2643589..ba60319 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *); > extern struct bio *bio_clone(struct bio *, gfp_t); > > extern void bio_init(struct bio *); > +extern void bio_reset(struct bio *); > > extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int); > extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *, > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h > index 293547e..40411e2 100644 > --- a/include/linux/blk_types.h > +++ b/include/linux/blk_types.h > @@ -59,6 +59,10 @@ struct bio { > unsigned int bi_seg_front_size; > unsigned int bi_seg_back_size; > > + /* > + * Everything starting with bi_max_vecs will be preserved by bio_reset() > + */ > + > unsigned int bi_max_vecs; /* max bvl_vecs we can hold */ > > atomic_t bi_cnt; /* pin count */ > @@ -93,6 +97,8 @@ struct bio { > struct bio_vec bi_inline_vecs[0]; > }; > > +#define BIO_RESET_BYTES offsetof(struct bio, bi_max_vecs) > + > /* > * bio flags > */ From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zimbra.linbit.com (zimbra.linbit.com [212.69.161.123]) by mail09.linbit.com (LINBIT Mail Daemon) with ESMTP id 7A6911055F57 for ; Thu, 26 Jul 2012 11:35:47 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by zimbra.linbit.com (Postfix) with ESMTP id 745641B4369 for ; Thu, 26 Jul 2012 11:35:47 +0200 (CEST) Received: from zimbra.linbit.com ([127.0.0.1]) by localhost (zimbra.linbit.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mwGi4YaoLbkG for ; Thu, 26 Jul 2012 11:35:47 +0200 (CEST) Received: from soda.linbit (tuerlsteher.linbit.com [86.59.100.100]) by zimbra.linbit.com (Postfix) with ESMTP id 5678F1B4368 for ; Thu, 26 Jul 2012 11:35:47 +0200 (CEST) Resent-Message-ID: <20120726093547.GY29767@soda.linbit> Received: from natasha.panasas.com (natasha.panasas.com [67.152.220.90]) by mail09.linbit.com (LINBIT Mail Daemon) with ESMTP id C989B1005431 for ; Wed, 25 Jul 2012 13:19:49 +0200 (CEST) Message-ID: <500FD63F.7050501@panasas.com> Date: Wed, 25 Jul 2012 14:19:27 +0300 From: Boaz Harrosh MIME-Version: 1.0 To: Kent Overstreet References: <1343160689-12378-1-git-send-email-koverstreet@google.com> <1343160689-12378-4-git-send-email-koverstreet@google.com> In-Reply-To: <1343160689-12378-4-git-send-email-koverstreet@google.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: axboe@kernel.dk, dm-devel@redhat.com, neilb@suse.de, linux-kernel@vger.kernel.org, linux-bcache@vger.kernel.org, mpatocka@redhat.com, vgoyal@redhat.com, yehuda@hq.newdream.net, tj@kernel.org, sage@newdream.net, agk@redhat.com, drbd-dev@lists.linbit.com Subject: Re: [Drbd-dev] [PATCH v4 03/12] block: Add bio_reset() List-Id: Coordination of development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 07/24/2012 11:11 PM, Kent Overstreet wrote: > Reusing bios is something that's been highly frowned upon in the past, > but driver code keeps doing it anyways. If it's going to happen anyways, > we should provide a generic method. > > This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c > was open coding it, by doing a bio_init() and resetting bi_destructor. > > Signed-off-by: Kent Overstreet > --- > fs/bio.c | 10 ++++++++++ > include/linux/bio.h | 1 + > include/linux/blk_types.h | 6 ++++++ > 3 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/fs/bio.c b/fs/bio.c > index 1c6c8b7..252e253 100644 > --- a/fs/bio.c > +++ b/fs/bio.c > @@ -261,6 +261,16 @@ void bio_init(struct bio *bio) > } > EXPORT_SYMBOL(bio_init); > > +void bio_reset(struct bio *bio) > +{ > + /* Clear all flags below BIO_OWNS_VEC */ > + unsigned long flags = bio->bi_flags & (~0UL << BIO_OWNS_VEC); > + Hey I have not seen these FLAGS thing before. Are these new? Anyway. Please NO!!! for one you need to put a big fat comment over at flags definitions. And two what happens when one adds a new flag. Is it reset or not reset? I'd rather you define a flags mask for those that need to be preserved, at header, plus a comment that any needed-to-be-preserved cross init flags, must be added to the mask. Never, ever, hide such crap so deep in the code. Boaz > + memset(bio, 0, BIO_RESET_BYTES); > + bio->bi_flags = flags|(1 << BIO_UPTODATE); > +} > +EXPORT_SYMBOL(bio_reset); > + > /** > * bio_alloc_bioset - allocate a bio for I/O > * @gfp_mask: the GFP_ mask given to the slab allocator > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 2643589..ba60319 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *); > extern struct bio *bio_clone(struct bio *, gfp_t); > > extern void bio_init(struct bio *); > +extern void bio_reset(struct bio *); > > extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int); > extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *, > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h > index 293547e..40411e2 100644 > --- a/include/linux/blk_types.h > +++ b/include/linux/blk_types.h > @@ -59,6 +59,10 @@ struct bio { > unsigned int bi_seg_front_size; > unsigned int bi_seg_back_size; > > + /* > + * Everything starting with bi_max_vecs will be preserved by bio_reset() > + */ > + > unsigned int bi_max_vecs; /* max bvl_vecs we can hold */ > > atomic_t bi_cnt; /* pin count */ > @@ -93,6 +97,8 @@ struct bio { > struct bio_vec bi_inline_vecs[0]; > }; > > +#define BIO_RESET_BYTES offsetof(struct bio, bi_max_vecs) > + > /* > * bio flags > */ From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756535Ab2GYLT4 (ORCPT ); Wed, 25 Jul 2012 07:19:56 -0400 Received: from natasha.panasas.com ([67.152.220.90]:34298 "EHLO natasha.panasas.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756422Ab2GYLTz (ORCPT ); Wed, 25 Jul 2012 07:19:55 -0400 Message-ID: <500FD63F.7050501@panasas.com> Date: Wed, 25 Jul 2012 14:19:27 +0300 From: Boaz Harrosh User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20111113 Thunderbird/8.0 MIME-Version: 1.0 To: Kent Overstreet CC: , , , , , , , , , , , Subject: Re: [PATCH v4 03/12] block: Add bio_reset() References: <1343160689-12378-1-git-send-email-koverstreet@google.com> <1343160689-12378-4-git-send-email-koverstreet@google.com> In-Reply-To: <1343160689-12378-4-git-send-email-koverstreet@google.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/24/2012 11:11 PM, Kent Overstreet wrote: > Reusing bios is something that's been highly frowned upon in the past, > but driver code keeps doing it anyways. If it's going to happen anyways, > we should provide a generic method. > > This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c > was open coding it, by doing a bio_init() and resetting bi_destructor. > > Signed-off-by: Kent Overstreet > --- > fs/bio.c | 10 ++++++++++ > include/linux/bio.h | 1 + > include/linux/blk_types.h | 6 ++++++ > 3 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/fs/bio.c b/fs/bio.c > index 1c6c8b7..252e253 100644 > --- a/fs/bio.c > +++ b/fs/bio.c > @@ -261,6 +261,16 @@ void bio_init(struct bio *bio) > } > EXPORT_SYMBOL(bio_init); > > +void bio_reset(struct bio *bio) > +{ > + /* Clear all flags below BIO_OWNS_VEC */ > + unsigned long flags = bio->bi_flags & (~0UL << BIO_OWNS_VEC); > + Hey I have not seen these FLAGS thing before. Are these new? Anyway. Please NO!!! for one you need to put a big fat comment over at flags definitions. And two what happens when one adds a new flag. Is it reset or not reset? I'd rather you define a flags mask for those that need to be preserved, at header, plus a comment that any needed-to-be-preserved cross init flags, must be added to the mask. Never, ever, hide such crap so deep in the code. Boaz > + memset(bio, 0, BIO_RESET_BYTES); > + bio->bi_flags = flags|(1 << BIO_UPTODATE); > +} > +EXPORT_SYMBOL(bio_reset); > + > /** > * bio_alloc_bioset - allocate a bio for I/O > * @gfp_mask: the GFP_ mask given to the slab allocator > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 2643589..ba60319 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *); > extern struct bio *bio_clone(struct bio *, gfp_t); > > extern void bio_init(struct bio *); > +extern void bio_reset(struct bio *); > > extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int); > extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *, > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h > index 293547e..40411e2 100644 > --- a/include/linux/blk_types.h > +++ b/include/linux/blk_types.h > @@ -59,6 +59,10 @@ struct bio { > unsigned int bi_seg_front_size; > unsigned int bi_seg_back_size; > > + /* > + * Everything starting with bi_max_vecs will be preserved by bio_reset() > + */ > + > unsigned int bi_max_vecs; /* max bvl_vecs we can hold */ > > atomic_t bi_cnt; /* pin count */ > @@ -93,6 +97,8 @@ struct bio { > struct bio_vec bi_inline_vecs[0]; > }; > > +#define BIO_RESET_BYTES offsetof(struct bio, bi_max_vecs) > + > /* > * bio flags > */