| Message ID | 1458225037-24155-2-git-send-email-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Good news about this interface, I just have a small suggestion in this patch: On Thu, Mar 17, 2016 at 10:30:29AM -0400, Brian Foster wrote: > From: Mike Snitzer <snitzer@redhat.com> > > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > --- > fs/block_dev.c | 20 ++++++++++++++++++++ > include/linux/blkdev.h | 5 +++++ > 2 files changed, 25 insertions(+) > > diff --git a/fs/block_dev.c b/fs/block_dev.c > index 826b164..375a2e4 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -497,6 +497,26 @@ long bdev_direct_access(struct block_device *bdev, struct blk_dax_ctl *dax) > } > EXPORT_SYMBOL_GPL(bdev_direct_access); > > +int blk_reserve_space(struct block_device *bdev, sector_t nr_sects) > +{ > + const struct block_device_operations *ops = bdev->bd_disk->fops; > + > + if (!ops->reserve_space) > + return -EOPNOTSUPP; > + return ops->reserve_space(bdev, nr_sects); > +} > +EXPORT_SYMBOL_GPL(blk_reserve_space); Wouldn't be better to have this function name standardized accordingly to the next one? Something like blk_set_reserved_space() maybe? > + > +int blk_get_reserved_space(struct block_device *bdev, sector_t *nr_sects) > +{ > + const struct block_device_operations *ops = bdev->bd_disk->fops; > + > + if (!ops->get_reserved_space) > + return -EOPNOTSUPP; > + return ops->get_reserved_space(bdev, nr_sects); > +} > +EXPORT_SYMBOL_GPL(blk_get_reserved_space); > + > /* > * pseudo-fs > */ > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index 413c84f..f212fe5 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -1664,6 +1664,8 @@ struct block_device_operations { > int (*getgeo)(struct block_device *, struct hd_geometry *); > /* this callback is with swap_lock and sometimes page table lock held */ > void (*swap_slot_free_notify) (struct block_device *, unsigned long); > + int (*reserve_space) (struct block_device *, sector_t); > + int (*get_reserved_space) (struct block_device *, sector_t *); > struct module *owner; > const struct pr_ops *pr_ops; > }; > @@ -1674,6 +1676,9 @@ extern int bdev_read_page(struct block_device *, sector_t, struct page *); > extern int bdev_write_page(struct block_device *, sector_t, struct page *, > struct writeback_control *); > extern long bdev_direct_access(struct block_device *, struct blk_dax_ctl *); > + > +extern int blk_reserve_space(struct block_device *, sector_t); > +extern int blk_get_reserved_space(struct block_device *, sector_t *); > #else /* CONFIG_BLOCK */ > > struct block_device; > -- > 2.4.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 21, 2016 at 01:08:29PM +0100, Carlos Maiolino wrote: > Hi, > > Good news about this interface, I just have a small suggestion in this patch: > > On Thu, Mar 17, 2016 at 10:30:29AM -0400, Brian Foster wrote: > > From: Mike Snitzer <snitzer@redhat.com> > > > > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > > --- > > fs/block_dev.c | 20 ++++++++++++++++++++ > > include/linux/blkdev.h | 5 +++++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/fs/block_dev.c b/fs/block_dev.c > > index 826b164..375a2e4 100644 > > --- a/fs/block_dev.c > > +++ b/fs/block_dev.c > > @@ -497,6 +497,26 @@ long bdev_direct_access(struct block_device *bdev, struct blk_dax_ctl *dax) > > } > > EXPORT_SYMBOL_GPL(bdev_direct_access); > > > > +int blk_reserve_space(struct block_device *bdev, sector_t nr_sects) > > +{ > > + const struct block_device_operations *ops = bdev->bd_disk->fops; > > + > > + if (!ops->reserve_space) > > + return -EOPNOTSUPP; > > + return ops->reserve_space(bdev, nr_sects); > > +} > > +EXPORT_SYMBOL_GPL(blk_reserve_space); > > Wouldn't be better to have this function name standardized accordingly to the > next one? Something like blk_set_reserved_space() maybe? Personally I see no point in wrappers like this. We don't add wrappers for ops methods in any other layers of the stack, filesystems are quite capable of checking if the method is available directly, so it seems pretty pointless to me... Cheers, Dave.
Re-add dm-devel@redhat.com, linux-block@vger.kernel.org to CC. On Tue, Mar 22, 2016 at 08:53:33AM +1100, Dave Chinner wrote: > On Mon, Mar 21, 2016 at 01:08:29PM +0100, Carlos Maiolino wrote: > > Hi, > > > > Good news about this interface, I just have a small suggestion in this patch: > > > > On Thu, Mar 17, 2016 at 10:30:29AM -0400, Brian Foster wrote: > > > From: Mike Snitzer <snitzer@redhat.com> > > > > > > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > > > --- > > > fs/block_dev.c | 20 ++++++++++++++++++++ > > > include/linux/blkdev.h | 5 +++++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/fs/block_dev.c b/fs/block_dev.c > > > index 826b164..375a2e4 100644 > > > --- a/fs/block_dev.c > > > +++ b/fs/block_dev.c > > > @@ -497,6 +497,26 @@ long bdev_direct_access(struct block_device *bdev, struct blk_dax_ctl *dax) > > > } > > > EXPORT_SYMBOL_GPL(bdev_direct_access); > > > > > > +int blk_reserve_space(struct block_device *bdev, sector_t nr_sects) > > > +{ > > > + const struct block_device_operations *ops = bdev->bd_disk->fops; > > > + > > > + if (!ops->reserve_space) > > > + return -EOPNOTSUPP; > > > + return ops->reserve_space(bdev, nr_sects); > > > +} > > > +EXPORT_SYMBOL_GPL(blk_reserve_space); > > > > Wouldn't be better to have this function name standardized accordingly to the > > next one? Something like blk_set_reserved_space() maybe? > > Personally I see no point in wrappers like this. We don't add > wrappers for ops methods in any other layers of the stack, > filesystems are quite capable of checking if the method is available > directly, so it seems pretty pointless to me... > I don't have too much of a preference, personally. I think these were slapped together fairly quickly just to get some kind of mechanism exposed. I was thinking more of combining them into a single method that takes a signed value for a reservation delta rather than an absolute value and simultaneously supports the ability to adjust or retrieve the current reservation. Unless I hear other thoughts or objections, I can probably clean that up and drop the wrappers for a subsequent post as I have some other fixes pending as well. Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/block_dev.c b/fs/block_dev.c index 826b164..375a2e4 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -497,6 +497,26 @@ long bdev_direct_access(struct block_device *bdev, struct blk_dax_ctl *dax) } EXPORT_SYMBOL_GPL(bdev_direct_access); +int blk_reserve_space(struct block_device *bdev, sector_t nr_sects) +{ + const struct block_device_operations *ops = bdev->bd_disk->fops; + + if (!ops->reserve_space) + return -EOPNOTSUPP; + return ops->reserve_space(bdev, nr_sects); +} +EXPORT_SYMBOL_GPL(blk_reserve_space); + +int blk_get_reserved_space(struct block_device *bdev, sector_t *nr_sects) +{ + const struct block_device_operations *ops = bdev->bd_disk->fops; + + if (!ops->get_reserved_space) + return -EOPNOTSUPP; + return ops->get_reserved_space(bdev, nr_sects); +} +EXPORT_SYMBOL_GPL(blk_get_reserved_space); + /* * pseudo-fs */ diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 413c84f..f212fe5 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1664,6 +1664,8 @@ struct block_device_operations { int (*getgeo)(struct block_device *, struct hd_geometry *); /* this callback is with swap_lock and sometimes page table lock held */ void (*swap_slot_free_notify) (struct block_device *, unsigned long); + int (*reserve_space) (struct block_device *, sector_t); + int (*get_reserved_space) (struct block_device *, sector_t *); struct module *owner; const struct pr_ops *pr_ops; }; @@ -1674,6 +1676,9 @@ extern int bdev_read_page(struct block_device *, sector_t, struct page *); extern int bdev_write_page(struct block_device *, sector_t, struct page *, struct writeback_control *); extern long bdev_direct_access(struct block_device *, struct blk_dax_ctl *); + +extern int blk_reserve_space(struct block_device *, sector_t); +extern int blk_get_reserved_space(struct block_device *, sector_t *); #else /* CONFIG_BLOCK */ struct block_device;