From patchwork Thu Sep 4 19:45:45 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zach Brown X-Patchwork-Id: 4847811 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 6D2479F2EC for ; Thu, 4 Sep 2014 19:45:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 1579C201D3 for ; Thu, 4 Sep 2014 19:45:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 979C7201CD for ; Thu, 4 Sep 2014 19:45:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755076AbaIDTpr (ORCPT ); Thu, 4 Sep 2014 15:45:47 -0400 Received: from tetsuo.zabbo.net ([50.193.208.193]:40141 "EHLO tetsuo.zabbo.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754433AbaIDTpq (ORCPT ); Thu, 4 Sep 2014 15:45:46 -0400 Received: from localhost (lenny.home.zabbo.net [192.168.242.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by tetsuo.zabbo.net (Postfix) with ESMTPSA id 94DB972004CA; Thu, 4 Sep 2014 12:45:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=zabbo.net; s=default; t=1409859944; bh=TuvXBbxPiWVMA6MSdXwmja2BSRqqCdv/pX7JnJVNlis=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=OsMUlS5tjRIj/ne8vvdDcknk45UY8Waf+2AKiYJeLK8YNDEUOMC3KELzoSgXoZv2p td6RPURK9iV04WXdZVHojKIrj5tQtDIQzjMqB/dE+EWjtkSokD5AfVrGTniXcQQJAb kbbyIvIGlLIK42HcjM/urQwLt6XR4ji1mOpC89yc= Date: Thu, 4 Sep 2014 12:45:45 -0700 From: Zach Brown To: Anand Jain Cc: David Sterba , linux-btrfs@vger.kernel.org, wangshilong1991@gmail.com Subject: Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer Message-ID: <20140904194545.GI15881@lenny.home.zabbo.net> References: <20130709202443.GJ18717@lenny.home.zabbo.net> <1373466615-30013-1-git-send-email-dsterba@suse.cz> <5408504C.1050203@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5408504C.1050203@oracle.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY, URIBL_WS_SURBL autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Sep 04, 2014 at 07:43:08PM +0800, Anand Jain wrote: > > > > + static __thread char _str[24]; > > This patch is causing segmentation fault as it reached maximum stack > depth on the sparc machine. Sigh. I guess it was inevitable that this would fail somewhere :). > Just earlier method of malloc is better ? No. Callers having to allocate per-argument buffers was insane. > unless we want to change the stack depth. Prooobably not. gcc still hasn't learned about registered format specifiers so we don't want to do that. How about just going back to the idea of using boring old macros for the format and args? I kind of remember that being discussed but I don't remember why we didn't go that route. Something like this, anyway.. (I even rememebered to fix the binaries that are mysteriously not built by default because reasons! Yeah!) - z From 3d132362f4c87b065b63cb38726a030db2277919 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Thu, 4 Sep 2014 12:32:00 -0700 Subject: [PATCH] btrfs-progs: use pretty printing macros The original pretty printing code was a mess and required callers to allocate and free buffers for each argument. The obvious fix would be to use glibc's dynamic format specifier registration, but gcc has yet to learn how to parse them when checking formats. It'd spew unknown specifier warnings if we add a pretty printing specifier. So you'd think we'd just use macros like everyone else. I don't remember the details now, but it seemed that people weren't excited by that. So we rolled some silly thread-local buffer for the pretty string for each argument. But that balloons the stack and crashes on sparc.. sure, fine. So we can go back to the dumb macros that are easy and work. Signed-off-by: Zach Brown --- btrfs-calc-size.c | 30 +++++++++++++++--------------- btrfs-fragments.c | 4 ++-- cmds-filesystem.c | 26 +++++++++++++------------- cmds-scrub.c | 4 ++-- mkfs.c | 4 ++-- utils.c | 35 ++++++++++++++++++++++++----------- utils.h | 18 +++++++++++------- 7 files changed, 69 insertions(+), 52 deletions(-) diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c index 501111c..bea7c81 100644 --- a/btrfs-calc-size.c +++ b/btrfs-calc-size.c @@ -324,6 +324,7 @@ static int calc_root_size(struct btrfs_root *tree_root, struct btrfs_key *key, int level; int ret = 0; int size_fail = 0; + u64 avg; root = btrfs_read_fs_root(tree_root->fs_info, key); if (IS_ERR(root)) { @@ -369,14 +370,15 @@ out_print: stat.total_clusters = 1; } + avg = stat.total_seeks ? stat.total_seek_len / stat.total_seeks : 0; + if (no_pretty || size_fail) { printf("\tTotal size: %Lu\n", stat.total_bytes); printf("\t\tInline data: %Lu\n", stat.total_inline); printf("\tTotal seeks: %Lu\n", stat.total_seeks); printf("\t\tForward seeks: %Lu\n", stat.forward_seeks); printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks); - printf("\t\tAvg seek len: %Lu\n", stat.total_seek_len / - stat.total_seeks); + printf("\t\tAvg seek len: %Lu\n", avg); print_seek_histogram(&stat); printf("\tTotal clusters: %Lu\n", stat.total_clusters); printf("\t\tAvg cluster size: %Lu\n", stat.total_cluster_size / @@ -389,25 +391,23 @@ out_print: (int)diff.tv_usec); printf("\tLevels: %d\n", level + 1); } else { - printf("\tTotal size: %s\n", pretty_size(stat.total_bytes)); - printf("\t\tInline data: %s\n", pretty_size(stat.total_inline)); + printf("\tTotal size: "PF"\n", PA(stat.total_bytes)); + printf("\t\tInline data: "PF"\n", PA(stat.total_inline)); printf("\tTotal seeks: %Lu\n", stat.total_seeks); printf("\t\tForward seeks: %Lu\n", stat.forward_seeks); printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks); - printf("\t\tAvg seek len: %s\n", stat.total_seeks ? - pretty_size(stat.total_seek_len / stat.total_seeks) : - pretty_size(0)); + printf("\t\tAvg seek len: "PF"\n", PA(avg)); print_seek_histogram(&stat); printf("\tTotal clusters: %Lu\n", stat.total_clusters); - printf("\t\tAvg cluster size: %s\n", - pretty_size((stat.total_cluster_size / + printf("\t\tAvg cluster size: "PF"\n", + PA((stat.total_cluster_size / stat.total_clusters))); - printf("\t\tMin cluster size: %s\n", - pretty_size(stat.min_cluster_size)); - printf("\t\tMax cluster size: %s\n", - pretty_size(stat.max_cluster_size)); - printf("\tTotal disk spread: %s\n", - pretty_size(stat.highest_bytenr - + printf("\t\tMin cluster size: "PF"\n", + PA(stat.min_cluster_size)); + printf("\t\tMax cluster size: "PF"\n", + PA(stat.max_cluster_size)); + printf("\tTotal disk spread: "PF"\n", + PA(stat.highest_bytenr - stat.lowest_bytenr)); printf("\tTotal read time: %d s %d us\n", (int)diff.tv_sec, (int)diff.tv_usec); diff --git a/btrfs-fragments.c b/btrfs-fragments.c index d03c2c3..fd45822 100644 --- a/btrfs-fragments.c +++ b/btrfs-fragments.c @@ -85,9 +85,9 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags, { double frag = (double)areas / (len / 4096) * 2; - fprintf(html, "

%s chunk starts at %lld, size is %s, %.2f%% used, " + fprintf(html, "

%s chunk starts at %lld, size is "PF", %.2f%% used, " "%.2f%% fragmented

\n", chunk_type(flags), start, - pretty_size(len), 100.0 * used / len, 100.0 * frag); + PA(len), 100.0 * used / len, 100.0 * frag); fprintf(html, "\n", name); } diff --git a/cmds-filesystem.c b/cmds-filesystem.c index 7e8ca95..d8b6938 100644 --- a/cmds-filesystem.c +++ b/cmds-filesystem.c @@ -210,11 +210,11 @@ static void print_df(struct btrfs_ioctl_space_args *sargs) struct btrfs_ioctl_space_info *sp = sargs->spaces; for (i = 0; i < sargs->total_spaces; i++, sp++) { - printf("%s, %s: total=%s, used=%s\n", + printf("%s, %s: total="PF", used="PF"\n", group_type_str(sp->flags), group_profile_str(sp->flags), - pretty_size(sp->total_bytes), - pretty_size(sp->used_bytes)); + PA(sp->total_bytes), + PA(sp->used_bytes)); } } @@ -326,18 +326,18 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices) total = device->total_devs; - printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf, + printf(" uuid: %s\n\tTotal devices %llu FS bytes used "PF"\n", uuidbuf, (unsigned long long)total, - pretty_size(device->super_bytes_used)); + PA(device->super_bytes_used)); list_sort(NULL, &fs_devices->devices, cmp_device_id); list_for_each(cur, &fs_devices->devices) { device = list_entry(cur, struct btrfs_device, dev_list); - printf("\tdevid %4llu size %s used %s path %s\n", + printf("\tdevid %4llu size "PF" used "PF" path %s\n", (unsigned long long)device->devid, - pretty_size(device->total_bytes), - pretty_size(device->bytes_used), device->name); + PA(device->total_bytes), + PA(device->bytes_used), device->name); devs_found++; } @@ -382,9 +382,9 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info, else printf("Label: none "); - printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf, + printf(" uuid: %s\n\tTotal devices %llu FS bytes used "PF"\n", uuidbuf, fs_info->num_devices, - pretty_size(calc_used_bytes(space_info))); + PA(calc_used_bytes(space_info))); for (i = 0; i < fs_info->num_devices; i++) { tmp_dev_info = (struct btrfs_ioctl_dev_info_args *)&dev_info[i]; @@ -396,10 +396,10 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info, continue; } close(fd); - printf("\tdevid %4llu size %s used %s path %s\n", + printf("\tdevid %4llu size "PF" used "PF" path %s\n", tmp_dev_info->devid, - pretty_size(tmp_dev_info->total_bytes), - pretty_size(tmp_dev_info->bytes_used), + PA(tmp_dev_info->total_bytes), + PA(tmp_dev_info->bytes_used), tmp_dev_info->path); } diff --git a/cmds-scrub.c b/cmds-scrub.c index 731c5c9..10a6692 100644 --- a/cmds-scrub.c +++ b/cmds-scrub.c @@ -151,8 +151,8 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p) printf("*** WARNING: memory allocation failed while scrubbing. " "results may be inaccurate\n"); - printf("\ttotal bytes scrubbed: %s with %llu errors\n", - pretty_size(p->data_bytes_scrubbed + p->tree_bytes_scrubbed), + printf("\ttotal bytes scrubbed: "PF" with %llu errors\n", + PA(p->data_bytes_scrubbed + p->tree_bytes_scrubbed), max(err_cnt, err_cnt2)); if (err_cnt || err_cnt2) { diff --git a/mkfs.c b/mkfs.c index 9de61e1..d649e03 100644 --- a/mkfs.c +++ b/mkfs.c @@ -1643,9 +1643,9 @@ raid_groups: BUG_ON(ret); printf("fs created label %s on %s\n\tnodesize %u leafsize %u " - "sectorsize %u size %s\n", + "sectorsize %u size "PF"\n", label, first_file, nodesize, leafsize, sectorsize, - pretty_size(btrfs_super_total_bytes(root->fs_info->super_copy))); + PA(btrfs_super_total_bytes(root->fs_info->super_copy))); btrfs_commit_transaction(trans, root); diff --git a/utils.c b/utils.c index 6c09366..0064587 100644 --- a/utils.c +++ b/utils.c @@ -709,8 +709,8 @@ int btrfs_prepare_device(int fd, char *file, int zero_end, u64 *block_count_ret, * optimization. */ if (discard_range(fd, 0, 0) == 0) { - fprintf(stderr, "Performing full device TRIM (%s) ...\n", - pretty_size(block_count)); + fprintf(stderr, "Performing full device TRIM ("PF") ...\n", + PA(block_count)); discard_blocks(fd, 0, block_count); } } @@ -1376,22 +1376,21 @@ out: return ret; } -static char *size_strs[] = { "", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"}; -int pretty_size_snprintf(u64 size, char *str, size_t str_bytes) +static float pretty_calc(u64 size, char **str) { int num_divs = 0; float fraction; + static char *size_strs[] = { + "", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", + }; - if (str_bytes == 0) - return 0; - - if( size < 1024 ){ + if (size < 1024) { fraction = size; num_divs = 0; } else { u64 last_size = size; num_divs = 0; - while(size >= 1024){ + while (size >= 1024) { last_size = size; size /= 1024; num_divs ++; @@ -1403,8 +1402,22 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes) } fraction = (float)last_size / 1024; } - return snprintf(str, str_bytes, "%.2f%s", fraction, - size_strs[num_divs]); + + *str = size_strs[num_divs]; + return fraction; +} + +float pretty_float(u64 size) +{ + char *str; + return pretty_calc(size, &str); +} + +char *pretty_suffix(u64 size) +{ + char *str; + pretty_calc(size, &str); + return str; } /* diff --git a/utils.h b/utils.h index fd25126..26c767b 100644 --- a/utils.h +++ b/utils.h @@ -71,13 +71,17 @@ int check_mounted_where(int fd, const char *file, char *where, int size, int btrfs_device_already_in_root(struct btrfs_root *root, int fd, int super_offset); -int pretty_size_snprintf(u64 size, char *str, size_t str_bytes); -#define pretty_size(size) \ - ({ \ - static __thread char _str[24]; \ - (void)pretty_size_snprintf((size), _str, sizeof(_str)); \ - _str; \ - }) +/* + * It's annoying that gcc hasn't yet figured out how to learn about + * formats added by register_printf_specifier(). If that were the case + * we could just add some %P type and be done with it. Some day.. + * + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47781 + */ +float pretty_float(u64 size); +char *pretty_suffix(u64 size); +#define PF "%.2f%s" +#define PA(x) pretty_float(x), pretty_suffix(x) int get_mountpt(char *dev, char *mntpt, size_t size); int btrfs_scan_block_devices(int run_ioctl);