WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen

To: Joe Jin <joe.jin@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen_blkif and some declares
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Sat, 6 Aug 2011 10:39:51 -0400
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Jens Axboe <jaxboe@xxxxxxxxxxxx>, Greg Marsden <greg.marsden@xxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, Annie Li <annie.li@xxxxxxxxxx>, Kurt C Hackel <KURT.HACKEL@xxxxxxxxxx>, Daniel Stodden <daniel.stodden@xxxxxxxxxx>
Delivery-date: Sat, 06 Aug 2011 07:40:58 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4E3A48FD.8060807@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4E3A486D.7060506@xxxxxxxxxx> <4E3A48FD.8060807@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Aug 04, 2011 at 03:23:41PM +0800, Joe Jin wrote:
> 
> Add remove_requested to xen_blkif and some declares.

By itself this patch is a bit strange. If you look in
Documentation/SubmittingPatches:
"2) Describe your changes.

Describe the technical detail of the change(s) your patch includes.
"

There is no really technical details. As in, why is this patch neccessary.

That document also states:
"3) Separate your changes.

Separate _logical changes_ into a single patch file.

For example, if your changes include both bug fixes and performance
enhancements for a single driver, separate those changes into two
or more patches.  If your changes include an API update, and a new
driver which uses that new API, separate those into two patches."


This patch by itself has no logical purpose - as in it does not fix a bug,
or add a new feature - it just plops a new element in a structure, provides
two function decleration of non-existing functions and a maccro that is not
used.

Soo.  Looking at the three patches I believe some reshuffling ought to be done.
If you recall my comments:


        >       case XenbusStateUnknown:
        > -             /* implies blkif_disconnect() via blkback_remove() */
        > +             /* implies xen_blkif_disconnect() via blkback_remove() 
*/

        Ok, that is not part of this patch. You should create another commit 
which
        does this type of cleanup and
        >               device_unregister(&dev->dev);
        >               break;
        >  
        > @@ -620,6 +778,8 @@ static void frontend_changed(struct xenbus_device 
*dev,
        >                                frontend_state);
        >               break;
        >       }
        > +
        > +     DPRINTK("%s: %s", dev->nodename, xenbus_strstate(dev->state));

        .. also for this.
        >  }
        >  

I am not sure if it is not clear, but what I meant that those two changes
(the comment rename and adding the DPRINKT) should be as a seperate
patch. So take those changes from patch #3 out and make a new patch.

Read on..
> 
> Signed-off-by: Joe Jin <joe.jin@xxxxxxxxxx>
> Cc: Daniel Stodden <daniel.stodden@xxxxxxxxxx>
> Cc: Jens Axboe <jaxboe@xxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Annie Li <annie.li@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> ---
>  drivers/block/xen-blkback/common.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/block/xen-blkback/common.h 
> b/drivers/block/xen-blkback/common.h
> index 9e40b28..acda757 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -49,6 +49,7 @@
>       pr_debug(DRV_PFX "(%s:%d) " fmt ".\n",  \
>                __func__, __LINE__, ##args)
>  
> +#define WPRINTK(fmt, args...) printk(KERN_WARNING "xen-blkback: " fmt, 
> ##args)

This is what I said in my review

        "What is 'WPRITNK' ? Can you use the the same type of printks
        as the rest? Also you have a space at the end. Make sure to
        run checkpath.pl"

which honeselty I should have been more specific. I meant that
you could just convert all the "WPRINTK" to what they define.

As in, sed/WPRINT/printk(KERN_WARNING/..

In essence, what I would like you to do is:

1). Read up on Documentation/SubmittingPatches
2). Squash this patch (except the declerations of the functions that are
    implemented in patch #3) into patch #2. The declerations of functions
    should be squashed in patch #3.
3). Replace in patch #3 all calls to WPRINTK with printk(KERN_WARNING.
4). Create a new patch that deals with the addition of DPRINTK
    and the update of the comment (see above).
5). The total should be three patches:
    a). This patch squashed with patch #2 (with the modification described in 
2).
    b). Patch #3 modified
    c). A new patch with the debug/comments modifications.
6). Make sure each patch compiles on its own.
7). Resend - or if you want to double check with me in case I've further
    comments - just send it to me privately.

>  
>  /* Not a real protocol.  Used to generate ring structs which contain
>   * the elements common to all protocols only.  This way we get a
> @@ -145,6 +146,7 @@ struct xen_blkif {
>       /* Back pointer to the backend_info. */
>       struct backend_info     *be;
>       /* Private fields. */
> +     bool                    remove_requested;
>       spinlock_t              blk_ring_lock;
>       atomic_t                refcnt;
>  
> @@ -198,6 +200,9 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction 
> xbt,
>  
>  struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);
>  
> +void xen_vbd_sync(struct xen_vbd *vbd);
> +void xen_blkback_close(struct xen_blkif *blkif);
> +
>  static inline void blkif_get_x86_32_req(struct blkif_request *dst,
>                                       struct blkif_x86_32_request *src)
>  {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel