Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim <minchan@kernel.org>]

Hello,

On Mon, Mar 09, 2015 at 03:48:56PM +0900, Sergey Senozhatsky wrote:
> On (03/09/15 11:21), Minchan Kim wrote:
> > > I was thinking for some time already about splitting stats that we
> > > export in two categories and, thus, two files: IO_stats and MM_stats.
> > > 
> > > zram<id>/io_stat
> > > 
> > > s*printf( num_reads, num_writes, failed_reads, failed_writes, etc.)
> > 
> > Some of it(ie, num_reads, num_writes) was duplicated with /dev/block/zramx/stat?
> > I know /dev/block/zramx/stat doesn't work now and I didn't check why it doesn't
> > work but I hope we make it work so remove duplicate stat, finally. :)
> > 
> 
> yes, I do recall looking into the issue some months ago. zramX/stat file hanled by block
> layer in various places. for example, in:
> 
> 	blk_finish_request(struct request *req, int error)
> 		blk_account_io_done():
> 
> doing
> 
>   part_stat_inc(cpu, part, ios[rw]);
>   part_stat_add(cpu, part, ticks[rw], duration);
>   part_round_stats(cpu, part);
>   part_dec_in_flight(part, rw);
> 
> 
> the problem here is that zram has several paths that may issue IO:
> -- usual zram_make_request()
> -- zram_slot_free_notify()
> -- zram_rw_page()
> 
> in zram_slot_free_notify() and zram_rw_page() we don't have request queue, request,
> etc. so it's a bit troubling.

I skim the code so I might miss something.

zram_slot_free_notify is just to free allocated space on zsmalloc so
it's not related to I/O operation so it would be okay if we handle
make_request and rw_page. Fortunately, they share core function
called by zram_bvec_rw.  So could we use generic_[start|end]_io_acct
in there? It seems we don't need request queue.

> 
> 
> besides, /sys/block/zramX/stat file exports totally different data:
> 
>  struct disk_stats {
>          unsigned long sectors[2];       /* READs and WRITEs */
>          unsigned long ios[2];
>          unsigned long merges[2];
>          unsigned long ticks[2];
>          unsigned long io_ticks;
>          unsigned long time_in_queue;
>  };
> 
> Documentation/block/stat.txt
> 
> Name            units         description
> ----            -----         -----------
> read I/Os       requests      number of read I/Os processed
> read merges     requests      number of read I/Os merged with in-queue I/O
> read sectors    sectors       number of sectors read
> read ticks      milliseconds  total wait time for read requests
> write I/Os      requests      number of write I/Os processed
> write merges    requests      number of write I/Os merged with in-queue I/O
> write sectors   sectors       number of sectors written
> write ticks     milliseconds  total wait time for write requests
> in_flight       requests      number of I/Os currently in flight
> io_ticks        milliseconds  total time this block device has been active
> time_in_queue   milliseconds  total wait time for all requests
> 
> 
> the only overlaps are num_read and num_write. so we will not be able to move all

When I read above, read/write ticks would be useful to us.

> (or any significant amount) of our IO stats to that file. that will force users
> to gather IO stats accross several files.

I'm not saying let's move all of I/O related stuff.
What I want is to remove duplicated stat if it is and enable zram/stats
so I hope we could use iostat/nmon to monitor zram I/O.

> 
> I'll take a look later today/tomorrow if I can do anything about it, but it seems
> that our own zramX/io_stat file would be simpler solution here. it does sound ugly,
> but it doesn't look so bad after all.

If it is really impossible or makes kernel complicated, I will agree with you.
Otherwise, I really want to see zram in iostat. :)

Thanks for looking this, Sergey!

-- 
Kind regards,
Minchan Kim