On Wed, May 11, 2016 at 11:31 AM, Jens Axboe <axboe@xxxxxxxxx> wrote:
> On 05/11/2016 11:25 AM, Michael Callahan wrote:
>>
>> On Tue, May 10, 2016 at 3:18 PM, Michael Callahan
>> <coder.callahan@xxxxxxxxx> wrote:
>>>
>>> On Tue, May 10, 2016 at 12:47 PM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
>>>>
>>>> Michael Callahan <michaelcallahan@xxxxxx> writes:
>>>>
>>>> Sorry for the segmented review.
>>>>
>>>>> diff --git a/block/bio.c b/block/bio.c
>>>>> index 807d25e..91b082d 100644
>>>>> --- a/block/bio.c
>>>>> +++ b/block/bio.c
>>>>> @@ -1678,27 +1678,29 @@ void bio_check_pages_dirty(struct bio *bio)
>>>>> }
>>>>> }
>>>>>
>>>>> -void generic_start_io_acct(int rw, unsigned long sectors,
>>>>> +void generic_start_io_acct(int sgrp, unsigned long sectors,
>>>>> struct hd_struct *part)
>>>>> {
>>>>> + const int rw = stat_group_to_rw(sgrp);
>>>>> int cpu = part_stat_lock();
>>>>>
>>>>> part_round_stats(cpu, part);
>>>>> - part_stat_inc(cpu, part, ios[rw]);
>>>>> - part_stat_add(cpu, part, sectors[rw], sectors);
>>>>> + part_stat_inc(cpu, part, ios[sgrp]);
>>>>> + part_stat_add(cpu, part, sectors[sgrp], sectors);
>>>>> part_inc_in_flight(part, rw);
>>>>>
>>>>> part_stat_unlock();
>>>>> }
>>>>> EXPORT_SYMBOL(generic_start_io_acct);
>>>>>
>>>>> -void generic_end_io_acct(int rw, struct hd_struct *part,
>>>>> +void generic_end_io_acct(int sgrp, struct hd_struct *part,
>>>>> unsigned long start_time)
>>>>> {
>>>>> unsigned long duration = jiffies - start_time;
>>>>> + const int rw = stat_group_to_rw(sgrp);
>>>>
>>>>
>>>> You modified these functions to take a stat group, then changed all
>>>> callers to turn an rw_flag into a stat group, and then turn right around
>>>> and convert that back into rw as the first order of business. Why can't
>>>> you do the conversion in generic_start/end_io_acct from rw to stat
>>>> group?
>>>>
>>>> Cheers,
>>>> Jeff
>>>
>>>
>>> stat_group_to_rw returns the data_dir and should probably be renamed
>>> as such. Really there were a couple of options here: 1) Per-cpu the
>>> partition in_flight counter. I mixed up this patch to play with and
>>> it appears to work fine except for the weirdness of code duplication
>>> in the raid driver. I'll post what I have in case anyone is
>>> interested in looking at md. Anyway, doing this would make the
>>> in_flight counter just take the sgrp and this conversion would not be
>>> necessary. 2) Extend generic_*_io_acct to take both rw and sgrp flags
>>> and just pass in both. This makes these functions not backwards
>>> compatible but also not compile if you try to use them that way. 3)
>>> Change the first parameter of these two functions to be the complete
>>> set of bi_rw or cmd_flags rather than just the data direction. I
>>> suppose this works as well as data_dir is a strict subset of the flags
>>> (& 1). However note that there does not appear to be a rw_data_dir()
>>> function to convert rw flags into a data direction.
>>>
>>> I'll see if there is any interest in per-cpu inflight counting as that
>>> seems cleanest to me.
>>>
>>> Michael
>>
>>
>> Is this what you had in mind? I'm not especially fond of the
>> 'rw_flags & 1' but I don't see anything cleaner available.
>>
>> void generic_start_io_acct(int rw_flags, unsigned long sectors,
>> struct hd_struct *part)
>> {
>> const int sgrp = rw_stat_group(rw_flags);
>> const int rw = rw_flags & 1;
>
>
> Just do:
>
> const int index = (rw_flags & REQ_WRITE) != 0;
>
> then you're not making any assumptions about what the WRITE bit is.
>
> --
> Jens Axboe
>
drivers/block/zram/zram_drv.c:zram_rw_page appears to be the only
.rw_page function that tracks stats via zram_bvec_rw. However there
is no rw_flags associated with this callback, just the data_dir. What
would be the best way to handle this case?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
