Re: [Xen-devel] [PATCH] xen-blkback: convert hole punching to discard request on loop devices

[Lukas Czerner <lczerner@xxxxxxxxxx>]

On Thu, 10 Nov 2011, Li Dongyang wrote:

> What am bit not sure is the secure_discard, since you've added the
> sec_discard to your tree,
> I'm wondering if we should treat hole punching as a secure discard to guest.
> Lukas, why u didn't add QUEUE_FLAG_SECDISCARD in your commit? is there
> something need to
> do with loop driver to have that flag? Thanks

Hi,

IIRC secure discard is almost the same as the "regular" discard, except
all sectors previously used for this data (moved by garbage collector)
are discarded as well. That is something only hardware can do, so I do
not think we should support this in loop driver. We would not be doing
anything different from "regular" discard anyway - that means just send
punch hole to the file system.

So I think that there is no reason secure discard should be supported by
loop driver, since there is nothing more "secure" about it.

Thanks!
-Lukas

> 
> On Wed, Nov 9, 2011 at 5:17 PM, Li Dongyang <lidongyang@xxxxxxxxxx> wrote:
> > On Wed, Nov 9, 2011 at 12:26 AM, Konrad Rzeszutek Wilk
> > <konrad.wilk@xxxxxxxxxx> wrote:
> >> On Mon, Nov 07, 2011 at 04:34:26PM +0800, Li Dongyang wrote:
> >>> As of dfaa2ef68e80c378e610e3c8c536f1c239e8d3ef, loop devices support
> >>> discard request now. We could just issue a discard request, and
> >>> the loop driver will punch the hole for us, so we don't need to touch
> >>> the internals of loop device and punch the hole ourselves, Thanks.
> >>
> >> Can I ask you to do two things:
> >>  1). Look in whether we can just eliminate the BLKIF_BACKEND_FILE 
> >> altogether?
> > I think we still need that flag, as for other type of backing dev, we
> > will figure it out and send back a EOPNOTSUPP.
> >>  2). Rebase this on top #devel/for-jens-3.3 
> >> (git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git)
> > no problem
> >>
> >> Thanks!
> >>>
> >>> Signed-off-by: Li Dongyang <lidongyang@xxxxxxxxxx>
> >>> ---
> >>>  drivers/block/xen-blkback/blkback.c |   21 +++------------------
> >>>  1 files changed, 3 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/block/xen-blkback/blkback.c 
> >>> b/drivers/block/xen-blkback/blkback.c
> >>> index 15ec4db..ad365a7 100644
> >>> --- a/drivers/block/xen-blkback/blkback.c
> >>> +++ b/drivers/block/xen-blkback/blkback.c
> >>> @@ -39,9 +39,6 @@
> >>>  #include <linux/list.h>
> >>>  #include <linux/delay.h>
> >>>  #include <linux/freezer.h>
> >>> -#include <linux/loop.h>
> >>> -#include <linux/falloc.h>
> >>> -#include <linux/fs.h>
> >>>
> >>>  #include <xen/events.h>
> >>>  #include <xen/page.h>
> >>> @@ -422,25 +419,13 @@ static void xen_blk_discard(struct xen_blkif 
> >>> *blkif, struct blkif_request *req)
> >>>       int status = BLKIF_RSP_OKAY;
> >>>       struct block_device *bdev = blkif->vbd.bdev;
> >>>
> >>> -     if (blkif->blk_backend_type == BLKIF_BACKEND_PHY)
> >>> -             /* just forward the discard request */
> >>> +     if (blkif->blk_backend_type == BLKIF_BACKEND_PHY ||
> >>> +         blkif->blk_backend_type == BLKIF_BACKEND_FILE)
> >>>               err = blkdev_issue_discard(bdev,
> >>>                               req->u.discard.sector_number,
> >>>                               req->u.discard.nr_sectors,
> >>>                               GFP_KERNEL, 0);
> >>> -     else if (blkif->blk_backend_type == BLKIF_BACKEND_FILE) {
> >>> -             /* punch a hole in the backing file */
> >>> -             struct loop_device *lo = bdev->bd_disk->private_data;
> >>> -             struct file *file = lo->lo_backing_file;
> >>> -
> >>> -             if (file->f_op->fallocate)
> >>> -                     err = file->f_op->fallocate(file,
> >>> -                             FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE,
> >>> -                             req->u.discard.sector_number << 9,
> >>> -                             req->u.discard.nr_sectors << 9);
> >>> -             else
> >>> -                     err = -EOPNOTSUPP;
> >>> -     } else
> >>> +     else
> >>>               err = -EOPNOTSUPP;
> >>>
> >>>       if (err == -EOPNOTSUPP) {
> >>> --
> >>> 1.7.7
> >>>
> >>>
> >>> _______________________________________________
> >>> Xen-devel mailing list
> >>> Xen-devel@xxxxxxxxxxxxxxxxxxx
> >>> http://lists.xensource.com/xen-devel
> >>
> >
> 

--

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