| Message ID | 20181026062435.21398-2-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: make sure discard/writesame bio is aligned with logical block size | expand |
> if (req_sects > UINT_MAX >> 9) > - req_sects = UINT_MAX >> 9; > + req_sects = (UINT_MAX >> 9) & ~bs_mask; Given that we have this same thing duplicated in write zeroes what about a documented helper?
On Fri, Oct 26, 2018 at 09:44:15AM +0200, Christoph Hellwig wrote: > > if (req_sects > UINT_MAX >> 9) > > - req_sects = UINT_MAX >> 9; > > + req_sects = (UINT_MAX >> 9) & ~bs_mask; > > Given that we have this same thing duplicated in write zeroes > what about a documented helper? IMO, using UINT_MAX & bs_mask is better because it is self-explanatory in the context. If we introduce one helper, it may not be easy to find a better name than UINT_MAX. thanks, Ming
On Sun, Oct 28, 2018 at 08:51:31AM +0800, Ming Lei wrote: > On Fri, Oct 26, 2018 at 09:44:15AM +0200, Christoph Hellwig wrote: > > > if (req_sects > UINT_MAX >> 9) > > > - req_sects = UINT_MAX >> 9; > > > + req_sects = (UINT_MAX >> 9) & ~bs_mask; > > > > Given that we have this same thing duplicated in write zeroes > > what about a documented helper? > > IMO, using UINT_MAX & bs_mask is better because it is self-explanatory > in the context. I don't think it is in any way. I understand it because I know the code, but there is nothing that documents why we do that.
On Sun, Oct 28, 2018 at 04:49:47PM +0100, Christoph Hellwig wrote: > On Sun, Oct 28, 2018 at 08:51:31AM +0800, Ming Lei wrote: > > On Fri, Oct 26, 2018 at 09:44:15AM +0200, Christoph Hellwig wrote: > > > > if (req_sects > UINT_MAX >> 9) > > > > - req_sects = UINT_MAX >> 9; > > > > + req_sects = (UINT_MAX >> 9) & ~bs_mask; > > > > > > Given that we have this same thing duplicated in write zeroes > > > what about a documented helper? > > > > IMO, using UINT_MAX & bs_mask is better because it is self-explanatory > > in the context. > > I don't think it is in any way. I understand it because I know the > code, but there is nothing that documents why we do that. Then how about introducing this helper? /* + * The max sectors one bio can handle is 'UINT_MAX >> 9' becasue + * bvec_iter.bi_size is defined as 'unsigned int', also it has to aligned + * to with logical block size which is minimum accepted unit by hardware. + */ +static inline unsigned int blk_max_allowed_max_secotrs(struct request_queue *q) +{ + return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9; +} + +/* Thanks, Ming
diff --git a/block/blk-lib.c b/block/blk-lib.c index bbd44666f2b5..aa3944946b2f 100644 --- a/block/blk-lib.c +++ b/block/blk-lib.c @@ -59,7 +59,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, if (!req_sects) goto fail; if (req_sects > UINT_MAX >> 9) - req_sects = UINT_MAX >> 9; + req_sects = (UINT_MAX >> 9) & ~bs_mask; end_sect = sector + req_sects;
Obviously the created discard bio has to be aligned with logical block size. Fixes: 744889b7cbb56a6 ("block: don't deal with discard limit in blkdev_issue_discard()") Reported-by: Rui Salvaterra <rsalvaterra@gmail.com> Cc: Rui Salvaterra <rsalvaterra@gmail.com> Cc: stable@vger.kernel.org Cc: Mike Snitzer <snitzer@redhat.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Xiao Ni <xni@redhat.com> Cc: Mariusz Dabrowski <mariusz.dabrowski@intel.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)