From patchwork Tue Dec 8 23:47:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Verma, Vishal L" X-Patchwork-Id: 7802811 Return-Path: X-Original-To: patchwork-linux-block@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 3AB799F71A for ; Tue, 8 Dec 2015 23:48:05 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4C6052042B for ; Tue, 8 Dec 2015 23:48:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 72B6D20434 for ; Tue, 8 Dec 2015 23:48:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751214AbbLHXsB (ORCPT ); Tue, 8 Dec 2015 18:48:01 -0500 Received: from mga14.intel.com ([192.55.52.115]:34539 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751033AbbLHXsA (ORCPT ); Tue, 8 Dec 2015 18:48:00 -0500 Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 08 Dec 2015 15:47:59 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,401,1444719600"; d="scan'208";a="702928996" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga003.jf.intel.com with ESMTP; 08 Dec 2015 15:47:59 -0800 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.31]) by FMSMSX106.amr.corp.intel.com ([169.254.5.39]) with mapi id 14.03.0248.002; Tue, 8 Dec 2015 15:47:58 -0800 From: "Verma, Vishal L" To: "Williams, Dan J" CC: "neilb@suse.com" , "linux-block@vger.kernel.org" , "jmoyer@redhat.com" , "linux-raid@vger.kernel.org" , "linux-scsi@vger.kernel.org" , "axboe@fb.com" , "linux-nvdimm@lists.01.org" , "James.Bottomley@hansenpartnership.com" Subject: Re: [PATCH v2 1/3] badblocks: Add core badblock management code Thread-Topic: [PATCH v2 1/3] badblocks: Add core badblock management code Thread-Index: AQHRJ7E6I5XEbI5IUk+YIXz8Um4d5J68Dh2AgAAIBgCABhhAgIAAAW4AgAAC4oCAACnAgA== Date: Tue, 8 Dec 2015 23:47:57 +0000 Message-ID: <1449618477.16905.126.camel@intel.com> References: <1448477013-9174-1-git-send-email-vishal.l.verma@intel.com> <1448477013-9174-2-git-send-email-vishal.l.verma@intel.com> <1449271801.27077.25.camel@HansenPartnership.com> <1449273524.16905.103.camel@intel.com> <87egewslfa.fsf@notabene.neil.brown.name> <1449608892.16905.121.camel@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.232.112.62] Content-ID: <9EEDFDEF98001E4994C8CE73EEB7C429@intel.com> MIME-Version: 1.0 Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable 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 Tue, 2015-12-08 at 13:18 -0800, Dan Williams wrote: > On Tue, Dec 8, 2015 at 1:08 PM, Verma, Vishal L > wrote: > > On Wed, 2015-12-09 at 08:03 +1100, NeilBrown wrote: > > > On Sat, Dec 05 2015, Verma, Vishal L wrote: > > > > > > > > > > > +int badblocks_clear(struct badblocks *bb, sector_t s, int > > > > > > sectors) > > > > > > +{ > > > > > [...] > > > > > > +#define DO_DEBUG 1 > > > > > > > > > > Why have this at all if it's unconditionally defined and > > > > > always > > > > > set. > > > > > > > > Neil - any reason or anything you had in mind for this? Or is it > > > > just an > > > > artifact and can be removed. > > > > > > Like the comment says: > > > > > >       /* Allow clearing via sysfs *only* for testing/debugging. > > >        * Normally only a successful write may clear a badblock > > >        */ > > > > > > The DO_DEBUG define and ifdefs are documentation identifying bits > > > of > > > code that should be removed when it all seems to be working. > > > Maybe now is a good time to remove that code. > > > > > Hm, I think it would be nice to continue to have the ability to > > clear > > badblocks using sysfs at least for a while more, as we test the > > various > > error handling paths for NVDIMMS (Dan, thoughts?). > > > > We could either remove it later or (I'm leaning towards) make it a > > config option similar to FAIL_MAKE_REQUEST and friends.. > > "later" as in before v4.5-rc1?  We can always carry this debug feature > locally for testing.  We don't want userspace growing ABI attachments > to this capability now that it's more than just md tooling that will > see this. Agreed. The following incremental patch removes sysfs support. All the latest badblocks patches can also be found at: git://git.kernel.org/pub/scm/linux/kernel/git/vishal/nvdimm.git gendisk-badblocks 8<----- From 5f0e7ac31d27a132f314106f1db33af22fde03ed Mon Sep 17 00:00:00 2001 From: Vishal Verma Date: Tue, 8 Dec 2015 16:28:31 -0700 Subject: [PATCH v4 4/3] badblocks: remove support for clearing via sysfs sysfs support for clearing badblocks was originally meant for testing only. With the move to generalize the interface, remove this support so that userspace doesn't start treating this as an ABI. Signed-off-by: Vishal Verma ---  block/badblocks.c | 15 ---------------  1 file changed, 15 deletions(-)   case 3:   if (newline != '\n') @@ -525,11 +515,6 @@ ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,   return -EINVAL;   }   - if (clear) { - badblocks_clear(bb, sector, length); - return len; - } -   if (badblocks_set(bb, sector, length, !unack))   return -ENOSPC;   else -- 2.5.0 diff --git a/block/badblocks.c b/block/badblocks.c index f0ac279..e5d2a91 100644 --- a/block/badblocks.c +++ b/block/badblocks.c @@ -503,16 +503,6 @@ ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,   int length;   char newline;   - /* Allow clearing via sysfs *only* for testing/debugging. -  * Normally only a successful write may clear a badblock -  */ - int clear = 0; - - if (page[0] == '-') { - clear = 1; - page++; - } -   switch (sscanf(page, "%llu %d%c", §or, &length, &newline)) {