From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161309AbcFGO7S (ORCPT ); Tue, 7 Jun 2016 10:59:18 -0400 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:14623 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932411AbcFGO7O (ORCPT ); Tue, 7 Jun 2016 10:59:14 -0400 Date: Tue, 7 Jun 2016 07:58:25 -0700 From: Shaohua Li To: Sitsofe Wheeler CC: , , , , , , , Dan Carpenter Subject: Re: [PATCH V2] block: correctly fallback for zeroout Message-ID: <20160607145824.GA84027@shli-mbp.local> References: <20160606223357.GA52883@shli-mbp.local> <20160607045049.GA10921@sucs.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160607045049.GA10921@sucs.org> User-Agent: Mutt/1.6.1 (2016-04-27) X-Originating-IP: [2620:10d:c090:180::1:e868] X-ClientProxiedBy: CO2PR20CA0027.namprd20.prod.outlook.com (10.163.96.37) To BY1PR15MB0055.namprd15.prod.outlook.com (10.161.97.149) X-MS-Office365-Filtering-Correlation-Id: 95e9adb0-2956-4455-f747-08d38ee4340c X-Microsoft-Exchange-Diagnostics: 1;BY1PR15MB0055;2:/LDhaNrGSrFZIzcrjQ1Yhkp4R0ulQnyjZ47lXB4LmywiAhoc8WNcJyKPtXHakKZN1JLUaTqTGjJrQoficCKir6Uz1uKm5X9c/RqupQV7HTOTg83CMpKJmiJQmtZVBowoVujhMJRnWMBZzXAmZWcTlGVvVCSvHG0cTSk46JKG15/236eeXbkZMoNc0FAz9XcO;3:yxXC8LjjZtKal9HkyLrgmVU8kQrBjGZTDB589nRH1Jj0ExW4PHEzeGjeFDRlqY4HkRLVDnbY1DoQaQFWOdG06ABKn5q5yswF36HDlH/3yWQNw8sEd9NvCJ0FdhiK6YdA X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY1PR15MB0055; X-Microsoft-Exchange-Diagnostics: 1;BY1PR15MB0055;25:6Fkcwby09FNWHfgFl7cUZBWreTMXdeePsbVxyVtT27Pe/Tjft0dfAn9nWvrIjgxt1H+SaSutw/mgSJ35CxbiXZEGGzp9tDNiJ66vPf4FL5ctsSpnx3VUpP0uIykMOMwSMtPNaDMBbxwgDqUANek6/0eVgXiw09q49wSMIkAK+z1nZaHXGGMeNTMqyzu6VPxNjl1CH4oM8DZJO/BtfGsO+G5dHVfxeCsI1JfkcSRpXsWCDEd1f1vuFVaucjn9p+KuAaf/1zs0Cdnqd8MHsmVABzjhxe+jYf0xBY6TzdcNGR8Z111QZqMed0sH7yBYPxOIZDqdsz/3Bh800Cfq0CMoMlAjLqguS7sFrcCbB7DM+xtu5sBmVeXaD4n/+IWx+Ajm0jo093KqSiPZyxnWf86Zq3GS2g5cw3Xe0v57RoeqSsSb4aRfeM1dN4I+SViYV+ovUOF//GOcI+qVto91zQyQof9iM9/roOukLanhy5PRKSBN2wrKBR666K8to6ewOuY7qPm/BVJ8K9OhVv8EpB3Uca9QyuVZ96CTJCYJJfc6t/ufILcOboyYMLvSYagdcQkcESeUhwar2L+X7ZAu07zYDxw+0tafsYbqGWn7/UXI9W/nKVfkoVUBi4JOLklNrRVpssSjaNjRrXAmbeqPU+W3qroG7Fa3C0zQSNw5SLeb9SGjErpMv/oBkbAJUHMcbdPyAzAGyr3ZDPxCe4Jnp2ce79Xnn+D1IW0dtpuh9guWaRuU9tfQ+ggaphovXD8yhc7ukNnphKnD4KjtcpTBA+3dN7DWfJUuyfzl1HBLHQnwQbrHbHmsA4ZBW6UFYfZIqfV/FV4dojk6RneKJSLZl+nQ3A== X-LD-Processed: 8ae927fe-1255-47a7-a2af-5f3a069daaa2,ExtAddr X-Microsoft-Exchange-Diagnostics: 1;BY1PR15MB0055;20:PvKOrcq1DUC//M6CV2QStpXyQPtYWcBQ11sqkOsB189bnm/1AGcripFwupvmtV8L2/WD6PbjQSToDzD4CFRhH6DMpa0tDF8HOlBgShYaRK8t785pOQNSytNY8nX9+BAF1A81vT3Obsvduu0JOsdDiDdY4b+GG0YXEdg9KO50rjg=;4:N/S4yaLwLOFvuLDIxcVzt8E6TVhdWjw1G2tcrd3g2OgTGPiGvHCwKDAjF7ySQ7WbNJl7DhGnNbPeqfspSo8UY+gn5zFfD6DfNg3xmFDC+cXks5uqYMEaJNuDUxCN4RncIYnovtnmFt+mzl+7bzXEqjTRVQnbbWQX2kZ0rFNqaibF824lmvy9Yv6b65gK+3pGP3cJMuhvvZcddDYLINZtKss0oSL8L94NXqEqGbk8P/AmWnXKVoJUcjVUq4RMg1OPLJ6XsMAbghuEOeJbda8ZCRaXD+EPz9y76xuqUuuaJdDKLJ0rgdmXbTlyyVkGsOc/u+yu0+v8KGMPo3jA8SsMU6kz8I5RRA7E9dqtKzhoeiujSpgoqovPGsOIPZHROiYQg3LaHUsqTbjPTQZbQTje569tieoHYiuSqcvYKvR/kOxgQK5OqIgCU7ZQhdAPMgONBAl+CSEQTCmZHA2YtS15bNj4LYj8rZ1O0huhcD2wJKE= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(67672495146484)(146099531331640)(201166117486090); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001);SRVR:BY1PR15MB0055;BCL:0;PCL:0;RULEID:;SRVR:BY1PR15MB0055; X-Forefront-PRVS: 09669DB681 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(199003)(189002)(24454002)(5008740100001)(101416001)(2950100001)(98436002)(81156014)(81166006)(15975445007)(47776003)(97756001)(86362001)(1411001)(33656002)(8676002)(77096005)(50986999)(92566002)(19580395003)(9686002)(19580405001)(54356999)(4326007)(76176999)(83506001)(2906002)(68736007)(50466002)(5004730100002)(23726003)(4001350100001)(42186005)(97736004)(1076002)(189998001)(6116002)(586003)(110136002)(105586002)(106356001)(46406003)(3826002);DIR:OUT;SFP:1102;SCL:1;SRVR:BY1PR15MB0055;H:shli-mbp.local;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BY1PR15MB0055;23:zO8fn3MpzK1C7xGAxCfWxvVDFb/TK167X8h0Sx3Le?= =?us-ascii?Q?ze56h2g8yBBD6rhNiFrWyLvPsxjkcMzVjQ1n3o0Oa8ESsT4cTyusmI88L3DY?= =?us-ascii?Q?250aNMw/a/qLgevS5OVCVBrO//FEjcmu1KJO+SSJZ2XXZ2udy6eM0M+I7+2u?= =?us-ascii?Q?QfrIWbYSu7AUPCeHFDBHJFTNVIqe39dzhxkSK9OtLjsVT+O8VoGwL3x0UmdW?= =?us-ascii?Q?QctUfORfTSsnqFTFc+ts1tpII3aikmzjG3IL5XdAUSw/ORG/IGZJAb/DXwOE?= =?us-ascii?Q?qURA2zkRgDiFnMbGucLQvzuuvDbO0+Grf+/pDN6Ak/vT+IHRYPU2nQMPhw3z?= =?us-ascii?Q?8oa+ilp5Dyx3tG6283bbe+x26B3FIaORTvQb2sp77fibEmHQHRSAYzni6sfR?= =?us-ascii?Q?s7zxxPdnHmh/GCHV6Z+EWcg/tZDPF9Z9zgZclssyNAhN5/3Ad4ZEMkrxRfdM?= =?us-ascii?Q?YzZSs0kIAqy4CPtB/G4hlndydHrrMhDs6QDzvwlaWZ/LZubRdSZLbLG6H3qj?= =?us-ascii?Q?qbEEoi3nPn1HYBU3kXs+g80y4tgi4hdjQrG7vnN97q29+35M6YgMX8duRzbO?= =?us-ascii?Q?ddGRuz/RdnxNx0fPNoRIbCUdQtV48Hz6smRe2r3TyZLyoMd1UR8Ew9d3CIR8?= =?us-ascii?Q?hQwXckU1ZpQ9fCkDv66bzI9BiFJyTvlJCrw+RMDrCyHxs11tolbirRZe4vPN?= =?us-ascii?Q?RGeBei1OHVf3510QHhJLRzEAic/dvYZJyUxDUZviDb+vZCFcylHpZhq3TaBh?= =?us-ascii?Q?IEa8usfvz+smfAFl1yNqbgRnWFxl2qRY8s9n2lWPjR4c7fXox1r+PthKXrMA?= =?us-ascii?Q?II/TC6XMdq2toqnC9QcL7uAacakeS8pjm7eb6wbnGCI1aWYMMK7dhSxcBBwc?= =?us-ascii?Q?mOcL3/f9gdhrhYbEeEktQjBd7r+baDgV4eyIEJ2yZ5lDbO8Dz63easEoBI71?= =?us-ascii?Q?3X2hcR7r0oIoHhID+RmeHph2kpY5hKwTHkexw22tUF4LuIadxfwzWY0pWsel?= =?us-ascii?Q?o9dXRP3Ji6gXQpdaTCPTkqIvEeLWwawQBdR9jdaFaThH2UDXIsHnhbZF8jUq?= =?us-ascii?Q?h4lx5+spyVgXq1bdQ1uPAytTFpAaBCQ36IKvAQ/18Z1MNfspClBqML4HpTm/?= =?us-ascii?Q?G3Qp6+NQ/NcU/MFqj79KBpAXAKMyr4LBvFrNYqWsllwFVxzrMB1dQ=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;BY1PR15MB0055;5:Y9B495HtSeOoKTO1VNi1qnhMY2LHgxGn3NyEPtNluxTy6B7k54MkD0JrTzgqzidr5tl35wrIJzdpRwkkY6ghmRzEog18vDcq/mwrGhv4iyfsPjqQPU4JGzx1dmXi4n16vWx+RdH8cnGKq+jqarCfrQ==;24:j4gCIBymEbpl9UUy3JT3/QSI45K5/dXQ84IIwBMgCF/p3KlV8KsqZJXULaAjiI25akiCSvauVW4ES8kawiX0luRXp4sehbr7o70f7n+JbwU=;7:UvA2wWx5yIfByfeuWrIzgyc4kKFcgcYGC3Plji4vjlgfCURQkTuNmN9FCo0AMzfN8iufZgGdNmH6d4M9T4AEvIqtVeXPr9Iq/wlLkNYce0D2Zfy9m7EpYBI1I3aCSpq10vfS5LNMNXezbdHhxHTjgSUhhM/P0l5OUIdFa03EjBL/fnNDoimFiCYKBNX0+kzYWe9CUL5JCIy4W2YsGOilK/38vAEIt1zeaxV2v4K3PFA=;20:6PP8vr07pMIzqjmw99uYRQ0hdF/xOvND1ge2Y4IFzURbbzUw/kz4zc3SXYmoccRvOnHtpGr+3lQkumd6Ppt9/OY4hHi8j3KHIuxFBON/JwvhllvXDdP/jTsfAwJvkL051dAjQJD5QgB4TEk8kDUannFUCM/tbEUMeNndzmixLvk= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2016 14:58:35.6637 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR15MB0055 X-OriginatorOrg: fb.com X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-06-07_07:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 07, 2016 at 05:50:49AM +0100, Sitsofe Wheeler wrote: > On Mon, Jun 06, 2016 at 03:33:58PM -0700, Shaohua Li wrote: > > blkdev_issue_zeroout try discard/writesame first, if they fail, zeroout > > fallback to regular write. The problem is discard/writesame doesn't > > return error for -EOPNOTSUPP, then zeroout can't do fallback and leave > > disk data not changed. zeroout should have guaranteed zero-fill > > behavior. > > > > https://bugzilla.kernel.org/show_bug.cgi?id=118581 > > > > V2: move the return value policy to blkdev_issue_discard and > > delete the policy for blkdev_issue_write_same (Martin) > > > > Cc: Sitsofe Wheeler > > Cc: Mike Snitzer > > Cc: Jens Axboe > > Cc: Martin K. Petersen > > Signed-off-by: Shaohua Li > > --- > > block/blk-lib.c | 49 +++++++++++++++++++++++++++++++------------------ > > 1 file changed, 31 insertions(+), 18 deletions(-) > > > > diff --git a/block/blk-lib.c b/block/blk-lib.c > > index 23d7f30..a3a26c8 100644 > > --- a/block/blk-lib.c > > +++ b/block/blk-lib.c > > @@ -84,6 +84,28 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > > } > > EXPORT_SYMBOL(__blkdev_issue_discard); > > > > +static int do_blkdev_issue_discard(struct block_device *bdev, sector_t sector, > > + sector_t nr_sects, gfp_t gfp_mask, unsigned long flags, > > + int *io_err) > > +{ > > + int type = REQ_WRITE | REQ_DISCARD; > > + struct bio *bio = NULL; > > + struct blk_plug plug; > > + int ret; > > + > > + if (flags & BLKDEV_DISCARD_SECURE) > > + type |= REQ_SECURE; > > + > > + blk_start_plug(&plug); > > + ret = __blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, type, > > + &bio); > > + if (!ret && bio) > > + *io_err = submit_bio_wait(type, bio); > > + blk_finish_plug(&plug); > > + > > + return ret; > > +} > > + > > /** > > * blkdev_issue_discard - queue a discard > > * @bdev: blockdev to issue discard for > > @@ -98,23 +120,12 @@ EXPORT_SYMBOL(__blkdev_issue_discard); > > int blkdev_issue_discard(struct block_device *bdev, sector_t sector, > > sector_t nr_sects, gfp_t gfp_mask, unsigned long flags) > > { > > - int type = REQ_WRITE | REQ_DISCARD; > > - struct bio *bio = NULL; > > - struct blk_plug plug; > > - int ret; > > + int ret, io_err; > > > > - if (flags & BLKDEV_DISCARD_SECURE) > > - type |= REQ_SECURE; > > - > > - blk_start_plug(&plug); > > - ret = __blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, type, > > - &bio); > > - if (!ret && bio) { > > - ret = submit_bio_wait(type, bio); > > - if (ret == -EOPNOTSUPP) > > - ret = 0; > > - } > > - blk_finish_plug(&plug); > > + ret = do_blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, > > + flags, &io_err); > > + if (!ret && io_err != -EOPNOTSUPP) > > + ret = io_err; > > Because io_err is always consulted if ret is not true shouldn't it be > explicitly initialized to 0 before the call to do_blkdev_issue_discard > (as do_blkdev_issue_discard will only set io_err if bio returned true)? > > Perhaps there's an argument that do_blkdev_issue_discard should always > set io_err on all its paths rather than just on errors in case the > caller hasn't initialized it - is there an existing kernel pattern for > this)? I didn't follow. io_err is only and always set when ret == 0. io_err is meanless if ret != 0, because that means the disk doesn't support discard and we don't dispatch discard IO. why should we initialized io_err to 0?