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

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

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.


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
(or any significant amount) of our IO stats to that file. that will force users
to gather IO stats accross several files.

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.


> > zram<id>/mm_stat
> > s*printf( orig_data_size, compr_data_size, mem_used_total, num_migrated, etc.)
> > 
> > 
> > 
> > so hoprefully in several years we can entirely remove ZRAM_ATTR_RO functions.
> > probably, first moving them under #ifdef CONFIG_OLD_ZRAM_STATS at some point
> > in the future.
> 
> Sounds good so we could warn for 1 or 2 years if users are about to use old stat
> and finally removes deprecated stat.

good.

> Please Cc util-linux zram-control peoples when you send patchset.
> 

ok.

	-ss