Messages in this thread | | | Subject | Re: [RFC PATCH] ceph: add remote object copy counter to fs client | From | Xiubo Li <> | Date | Wed, 27 Oct 2021 14:46:09 +0800 |
| |
On 10/26/21 11:31 PM, Luís Henriques wrote: > On Tue, Oct 26, 2021 at 07:40:51AM -0400, Jeff Layton wrote: >> On Tue, 2021-10-26 at 11:05 +0800, Xiubo Li wrote: >>> On 10/22/21 1:30 AM, Patrick Donnelly wrote: >>>> On Thu, Oct 21, 2021 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote: >>>>> On Thu, 2021-10-21 at 12:18 -0400, Patrick Donnelly wrote: >>>>>> On Thu, Oct 21, 2021 at 11:44 AM Jeff Layton <jlayton@kernel.org> wrote: >>>>>>> On Thu, 2021-10-21 at 09:52 -0400, Patrick Donnelly wrote: >>>>>>>> On Wed, Oct 20, 2021 at 12:27 PM Jeff Layton <jlayton@kernel.org> wrote: >>>>>>>>> On Wed, 2021-10-20 at 15:37 +0100, Luís Henriques wrote: >>>>>>>>>> This counter will keep track of the number of remote object copies done on >>>>>>>>>> copy_file_range syscalls. This counter will be filesystem per-client, and >>>>>>>>>> can be accessed from the client debugfs directory. >>>>>>>>>> >>>>>>>>>> Cc: Patrick Donnelly <pdonnell@redhat.com> >>>>>>>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>>>>>>>>> --- >>>>>>>>>> This is an RFC to reply to Patrick's request in [0]. Note that I'm not >>>>>>>>>> 100% sure about the usefulness of this patch, or if this is the best way >>>>>>>>>> to provide the functionality Patrick requested. Anyway, this is just to >>>>>>>>>> get some feedback, hence the RFC. >>>>>>>>>> >>>>>>>>>> Cheers, >>>>>>>>>> -- >>>>>>>>>> Luís >>>>>>>>>> >>>>>>>>>> [0] https://github.com/ceph/ceph/pull/42720 >>>>>>>>>> >>>>>>>>> I think this would be better integrated into the stats infrastructure. >>>>>>>>> >>>>>>>>> Maybe you could add a new set of "copy" stats to struct >>>>>>>>> ceph_client_metric that tracks the total copy operations done, their >>>>>>>>> size and latency (similar to read and write ops)? >>>>>>>> I think it's a good idea to integrate this into "stats" but I think a >>>>>>>> local debugfs file for some counters is still useful. The "stats" >>>>>>>> module is immature at this time and I'd rather not build any qa tests >>>>>>>> (yet) that rely on it. >>>>>>>> >>>>>>>> Can we generalize this patch-set to a file named "op_counters" or >>>>>>>> similar and additionally add other OSD ops performed by the kclient? >>>>>>>> >>>>>>> Tracking this sort of thing is the main purpose of the stats code. I'm >>>>>>> really not keen on adding a whole separate set of files for reporting >>>>>>> this. >>>>>> Maybe I'm confused. Is there some "file" which is already used for >>>>>> this type of debugging information? Or do you mean the code for >>>>>> sending stats to the MDS to support cephfs-top? >>>>>> >>>>>>> What's the specific problem with relying on the data in debugfs >>>>>>> "metrics" file? >>>>>> Maybe no problem? I wasn't aware of a "metrics" file. >>>>>> >>>>> Yes. For instance: >>>>> >>>>> # cat /sys/kernel/debug/ceph/*/metrics >>>>> item total >>>>> ------------------------------------------ >>>>> opened files / total inodes 0 / 4 >>>>> pinned i_caps / total inodes 5 / 4 >>>>> opened inodes / total inodes 0 / 4 >>>>> >>>>> item total avg_lat(us) min_lat(us) max_lat(us) stdev(us) >>>>> ----------------------------------------------------------------------------------- >>>>> read 0 0 0 0 0 >>>>> write 5 914013 824797 1092343 103476 >>>>> metadata 79 12856 1572 114572 13262 >>>>> >>>>> item total avg_sz(bytes) min_sz(bytes) max_sz(bytes) total_sz(bytes) >>>>> ---------------------------------------------------------------------------------------- >>>>> read 0 0 0 0 0 >>>>> write 5 4194304 4194304 4194304 20971520 >>>>> >>>>> item total miss hit >>>>> ------------------------------------------------- >>>>> d_lease 11 0 29 >>>>> caps 5 68 10702 >>>>> >>>>> >>>>> I'm proposing that Luis add new lines for "copy" to go along with the >>>>> "read" and "write" ones. The "total" counter should give you a count of >>>>> the number of operations. >>>> Okay that makes more sense! >>>> >>>> Side note: I am a bit horrified by how computer-unfriendly that >>>> table-formatted data is. >>> Any suggestion to improve this ? >>> >>> How about just make the "metric" file writable like a switch ? And as >>> default it will show the data as above and if tools want the >>> computer-friendly format, just write none-zero to it, then show raw data >>> just like: >>> >>> # cat /sys/kernel/debug/ceph/*/metrics >>> opened_files:0 >>> pinned_i_caps:5 >>> opened_inodes:0 >>> total_inodes:4 >>> >>> read_latency:0,0,0,0,0 >>> write_latency:5,914013,824797,1092343,103476 >>> metadata_latency:79,12856,1572,114572,13262 >>> >>> read_size:0,0,0,0,0 >>> write_size:5,4194304,4194304,4194304,20971520 >>> >>> d_lease:11,0,29 >>> caps:5,68,10702 >>> >>> >> I'd rather not multiplex the output of this file based on some input. >> That would also be rather hard to do -- write() and read() are two >> different syscalls, so you'd need to track a bool (or something) across >> them somehow. >> >> Currently, I doubt there are many scripts in the field that scrape this >> info and debugfs is specifically excluded from ABI concerns. If we want >> to make it more machine-readable (which sounds like a good thing), then >> I suggest we just change the output to something like what you have >> above and not worry about preserving the "legacy" output. > Ok, before submitting any new revision of this patch I should probably > clean this up. I can submit a patch to change the format to what Xiubo is > proposing. Obviously, that patch will also need to document what all > those fields actually mean. > > Alternatively, the metrics file could be changed into a directory and have > 4 different files, one per each section: > > metrics/ > |- files <-- not sure how to name the 1st section > |- latency > |- size > \- caps > > Each of these files would then have the header but, since it's a single > header, parsing it in a script would be pretty easy. The advantage is > that this would be self-documented (with filenames and headers).
This sounds good to me.
> > Cheers, > -- > Luís >
| |