From patchwork Fri Jul 2 20:00:49 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikulas Patocka X-Patchwork-Id: 57772 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id AA35B1007D5 for ; Sat, 3 Jul 2010 06:01:00 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759918Ab0GBUA6 (ORCPT ); Fri, 2 Jul 2010 16:00:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47211 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759729Ab0GBUA5 (ORCPT ); Fri, 2 Jul 2010 16:00:57 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o62K0u1A017666 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 2 Jul 2010 16:00:57 -0400 Received: from hs20-bc2-1.build.redhat.com (hs20-bc2-1.build.redhat.com [10.10.28.34]) by int-mx08.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o62K0ovb007839 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 2 Jul 2010 16:00:50 -0400 Received: from hs20-bc2-1.build.redhat.com (localhost.localdomain [127.0.0.1]) by hs20-bc2-1.build.redhat.com (8.13.1/8.13.1) with ESMTP id o62K0oNc013360; Fri, 2 Jul 2010 16:00:50 -0400 Received: from localhost (mpatocka@localhost) by hs20-bc2-1.build.redhat.com (8.13.1/8.13.1/Submit) with ESMTP id o62K0n4j013354; Fri, 2 Jul 2010 16:00:49 -0400 X-Authentication-Warning: hs20-bc2-1.build.redhat.com: mpatocka owned process doing -bs Date: Fri, 2 Jul 2010 16:00:49 -0400 (EDT) From: Mikulas Patocka X-X-Sender: mpatocka@hs20-bc2-1.build.redhat.com To: Mike Snitzer cc: dm-devel@redhat.com, Alasdair G Kergon , linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org Subject: Re: [PATCH 4/4] Support discard if at least one underlying device supports it In-Reply-To: Message-ID: References: <20100702181430.GD26916@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.21 Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Fri, 2 Jul 2010, Mikulas Patocka wrote: > > As we discussed, we have a challenge where we need DM to avoid issuing > > a barrier before the discard IFF a target doesn't support the discard > > (which the barrier is paired with). > > > > My understanding is that blkdev_issue_discard() only cares if the > > discard was supported. Barrier is used just to decorate the discard > > (for correctness). So by returning -EOPNOTSUPP we're saying the discard > > isn't supported; we're not making any claims about the implict barrier, > > so best to avoid the barrier entirely. > > > > Otherwise we'll be issuing unnecessary barriers (and associated > > performance loss). > > > > So yet another TODO item... Anyway: > > > > Acked-by: Mike Snitzer > > Unnecessary barriers are issued anyway. With each freed extent. > > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that > unmap to disk. And this in loop for all extents in > "release_blocks_on_commit". > > One idea behind "discard barriers" was to submit a discard request and not > wait for it. Then the request would need a barrier so that it doesn't get > reordered with further writes (that may potentially write to the same area > as the discarded area). But discard isn't used this way anyway, > sb_issue_discard waits for completion, so the barrier isn't needed. > > Even if ext4 developers wanted asynchronous discard requests, they should > fire all the discards at once and then submit one zero-sized barrier. Not > barrier with each discard request. > > This is up to ext4 developers to optimize and remove the barriers and we > can't do anything with it. Just send "SYNCHRONIZE > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants... > > Mikulas BTW. I understand that the current dm implementation will send two useless consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part of the device that doesn't support it. But the problem is that when you use discard on a part of the device that supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands --- they are useless for functionality, but mandated by the barrier specification. The fix is supposedly this: --- include/linux/blkdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h =================================================================== --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h 2010-07-02 21:59:21.000000000 +0200 +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h 2010-07-02 21:59:37.000000000 +0200 @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc block <<= (sb->s_blocksize_bits - 9); nr_blocks <<= (sb->s_blocksize_bits - 9); return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL, - BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER); + BLKDEV_IFL_WAIT); } extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);