Re: [PATCH 1/7] ѕd: split sd_setup_discard_cmnd
On Tue, Mar 28, 2017 at 08:05:09AM -0600, ax...@kernel.dk wrote: > > Although I know this is an issue in the existing code and not something > > introduced by you: please consider using logical_to_sectors() instead of > > open-coding this function. Otherwise this patch looks fine to me. > > The downside of doing that is that we still call ilog2() twice, which > sucks. Would be faster to cache ilog2(sector_size) and use that in the > shift calculation. I suspect that gcc is smart enough to optimize it away. That beeing said while this looks like a nice cleanup this patch is just supposed to move code, so I'd rather not add the change here and leave it for a separate submission.
Re: [PATCH 1/7] ѕd: split sd_setup_discard_cmnd
On Mon, Mar 27 2017, Bart Van Assche wrote: > On Mon, 2017-03-20 at 16:43 -0400, Christoph Hellwig wrote: > > + u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9); > > + u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9); > > Although I know this is an issue in the existing code and not something > introduced by you: please consider using logical_to_sectors() instead of > open-coding this function. Otherwise this patch looks fine to me. The downside of doing that is that we still call ilog2() twice, which sucks. Would be faster to cache ilog2(sector_size) and use that in the shift calculation. -- Jens Axboe
Re: [PATCH 1/7] ѕd: split sd_setup_discard_cmnd
On Mon, 2017-03-20 at 16:43 -0400, Christoph Hellwig wrote: > + u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9); > + u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9); Although I know this is an issue in the existing code and not something introduced by you: please consider using logical_to_sectors() instead of open-coding this function. Otherwise this patch looks fine to me. Thanks, Bart.