Received: by 2002:a05:6358:1087:b0:cb:c9d3:cd90 with SMTP id j7csp904124rwi; Fri, 14 Oct 2022 10:03:11 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4/vofBE+8BFY9ojZNhpgX31DADCrV80xTFRe8DfP0CIKy7ZXaGRkgHyGnPdwHar509pd3n X-Received: by 2002:a05:6402:5202:b0:45c:9525:9bc4 with SMTP id s2-20020a056402520200b0045c95259bc4mr5191031edd.380.1665766990816; Fri, 14 Oct 2022 10:03:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1665766990; cv=none; d=google.com; s=arc-20160816; b=1FAarbpV1gzFz8Bahp0vKIqudHyDCYF8pfeD+JqJnQJwxw7GO+qt0/kyfyPyMtsXGA NfaN3t/QBI2tqocp5YlIXG3224eAAba9AoBLfzPDVt/glhGpBWgZSUtFz9C3thlKsewj JfZTiEv2m/71mRXKan6MTpOw85QPT4GIgUlKkIzbubQiRlVaTwGsLP4oLD8N/blnZHjK CWf6hVR7rjhTt5mk7j6mX3dv7lm6g3HGik96Zn3hwviqsq/7cReRHXTBBP2OfyzGmHKR UMpqQ6rho/Hx2/Q4Rl3ahKnBH2JnTFMOwOAcTZPAmzVofI+p7feJ2WzsnJ9qVjYMm/T3 tsVQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to:subject :from:references:cc:to:content-language:user-agent:mime-version:date :message-id; bh=PgLEvPOHq9Nsuzhx3QMShM7SQ9o6Q8Xf9M9jps2YqXQ=; b=jbFpU/jAuyB94+NZ057RjFZp6TBBgRJxZlUYQT7EN1SVfD0NCuAW8LDCNL6ByDeBW2 PWWCy3bEiofXAlWNOWMhKYout+20DcbPAMPlnQS6arMPF0TOef9LYzGm4cjdWCkENVAS VpWBgXRuTRauj4ccm7wG5eGSWkO/d3WHRxAoH4WUEMKPm0KSKExF3Dwx0EWe/GSqe8Eh 1KtWIdPDNmEPaTeNRZQqMOt+mwnH812o9XzWEWkaL7PDutK9AHQu767dLWqiSklIknB/ ArPs5Jx2hu8EPCfAEif9Qnh34expG2v9IjSpk09xTD3F1aII8eSqWnUT9J9qaGzf/3nd Xyiw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id d21-20020a170906345500b0078d2f0bf546si2198008ejb.186.2022.10.14.10.02.39; Fri, 14 Oct 2022 10:03:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231136AbiJNQon (ORCPT + 99 others); Fri, 14 Oct 2022 12:44:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55422 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230523AbiJNQok (ORCPT ); Fri, 14 Oct 2022 12:44:40 -0400 Received: from vps-vb.mhejs.net (vps-vb.mhejs.net [37.28.154.113]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 165F9A476; Fri, 14 Oct 2022 09:44:34 -0700 (PDT) Received: from MUA by vps-vb.mhejs.net with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1ojNnU-0004xS-0P; Fri, 14 Oct 2022 18:44:32 +0200 Message-ID: <220d39ad-11cc-338f-806e-293ac43b5021@maciej.szmigiero.name> Date: Fri, 14 Oct 2022 18:44:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Content-Language: en-US, pl-PL To: Damien Le Moal Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org References: <24a48f71-8a79-6311-1e43-494df0458a32@opensource.wdc.com> <7ecf20b7-794a-39d8-0b03-8f19d9167efd@maciej.szmigiero.name> <28712bad-8215-4246-7370-42d204488aa3@opensource.wdc.com> <7cf5744e-78ec-79c3-98af-2a716167ea1a@opensource.wdc.com> <31f8c4d1-1575-e64d-f42a-ce864e060975@maciej.szmigiero.name> From: "Maciej S. Szmigiero" Subject: Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 8.10.2022 00:41, Damien Le Moal wrote: > On 10/7/22 23:14, Maciej S. Szmigiero wrote: >> On 7.10.2022 00:53, Damien Le Moal wrote: >>> On 10/7/22 07:20, Damien Le Moal wrote: >>>> On 10/6/22 22:06, Maciej S. Szmigiero wrote: >>>>> On 6.10.2022 01:38, Damien Le Moal wrote: >>>>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote: >>>>>>> From: "Maciej S. Szmigiero" >>>>>>> >>>>>>> Currently, if one wants to make use of FUA support in libata it is >>>>>>> necessary to provide an explicit kernel command line parameter in order to >>>>>>> enable it (for drives that report such support). >>>>>>> >>>>>>> In terms of Git archaeology: FUA support was enabled by default in early >>>>>>> libata versions but was disabled soon after. >>>>>>> Since then there were a few attempts to enable this support by default: >>>>>>> [1] (for NCQ drives only), [2] (for all drives). >>>>>>> However, the second change had to be reverted after a report came of >>>>>>> an incompatibility with the HDD in 2011 Mac Mini. >>>>>>> >>>>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache >>>>>>> flush for every request that have this flag set. >>>>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that >>>>>>> supports LBA48 and so these days should be pretty widespread let's provide >>>>>>> an ability to enable it by default in Kconfig. >>>>>> >>>>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So >>>>>> I do not see the need to add yet another config option. >>>>> >>>>> A specific Kconfig option is more structured than a free-form >>>>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems >>>>> to be widely supported across arches). >>>>> >>>>> That's why there is a lot (100+) of similar Kconfig default-changing >>>>> options, a quick sample of these (in no particular order): >>>>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM, >>>>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE, >>>>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX, >>>>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME, >>>>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ... >>>>> >>>>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY, >>>>> so it's not like a person performing kernel configuration is >>>>> overloaded with questions here. >>>>> >>>>> But at the same time, I respect your decision as a maintainer of >>>>> this code. >>>> >>>> I am not dead set on pushing back on this, but as usual, whenever we can >>>> avoid adding config options, we should. Given that libata has had fua >>>> disabled forever, I am not convinced yet that there is a strong need for >>>> that new option. But if distros prefer the config option approach, we can >>>> make that happen. >>>> >>>> If anything, I would be tempted to switch fua support to on by default >>>> after some time if we do not get many reports of broken drives. You did >>>> mention that old mac minis drives did not like it, but these are not even >>>> blacklisted in libata-scsi. They should. Only one model of maxtor drives >>>> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist >>>> of drives not liking fua. Without that in place, switching to on by >>>> default as your config option allows could break many (old) systems. >>> >>> To be extra clear, I think that this fua module parameter is silly. If a >>> drive says it supports fua, we should use it and not have a global >>> parameter to disable it for all drives. So no config option needed for it. >>> >>> That is also why I am not keen on taking that config option. It is not >>> really improving anything at all and would prefer nuking the fua module >>> argument and have a proper blacklisting of buggy drives. >>> >>> But such a change is painful as we'll be able to update the blacklist with >>> users getting corrupted FSes on buggy drives. The time may have come to do >>> this change though as the number of buggy drives out there is hopefully >>> small enough now. >> >> So your proposal is basically to switch the existing fua option default >> to "on" and deal with the fallout (hopefully minimal) by blacklisting >> misbehaving drives as they get reported, right? > > Yes. The risk though is that if the fallout are not minimal and we get too > many bug reports, we'll likely have to revert. So this needs to be > attempted early at the beginning of a cycle to get plenty of testing. > >> In this case, my vote would be to still keep the "libata.fua" parameter >> available (at least for the time being) so people have some way of >> working broken drives around without having to recompile their kernel >> (maybe also print a kernel log message if libata.fua=0 is provided asking >> people to report these drive models to linux-ide@). > > If we add a proper "nofua" horkage flag to blacklist buggy drives, we need > to move the fua parameter to be an argument of the force parameter so that > disabling fua can be done per drive (port) or for all drives, similarly to > other tweak (noncq, nodmalog, etc) So would you like an updated patch set or do you prefer to do the changes yourself? Thanks, Maciej