From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:38078 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752575AbeAFJDN (ORCPT ); Sat, 6 Jan 2018 04:03:13 -0500 Date: Sat, 6 Jan 2018 17:02:54 +0800 From: Ming Lei To: Christoph Hellwig Cc: Jens Axboe , linux-block@vger.kernel.org, reddot.rocks@gmail.com, linux-fsdevel@vger.kernel.org, Stefan Haberland , Jan Hoeppner , Martin Schwidefsky , Heiko Carstens , linux-s390@vger.kernel.org Subject: Re: [PATCH] block: loop: remove & invalidate partitions when detaching Message-ID: <20180106090244.GA13472@ming.t460p> References: <20180104103924.8814-1-ming.lei@redhat.com> <20180104123803.GA8437@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180104123803.GA8437@infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Jan 04, 2018 at 04:38:03AM -0800, Christoph Hellwig wrote: > On Thu, Jan 04, 2018 at 06:39:24PM +0800, Ming Lei wrote: > > When detaching loop disk, neither we remove loop partitions, nor > > invalidate them. This way may cause data loss, and often confuse > > people. > > > > This patch fixes the above issue by removing & invalidating loop > > partitions in loop_clr_fd(), > > Please add a comment explaining the call (e.g. the weird loop > case of keeping unbound devices around). OK. > > Also dasd seems to implement this functionality by iterating over > all partitions and then calling ioctl_by_bdev(bdev, BLKPG, ..) on > it. Please switch it to you new helper assuming there is a good > reason to even do this to start with. CCing the maintainers to > confirm that. This patch doesn't consider to hold bdev lock, and loop's case can be a bit complicated since loop_clr_fd() can be called in release path. I will investigate a bit if it is possible to figure out a general helper to cover both. > > And while you'e at it split the block and loop bits into separate > patches. OK. > > > +void del_gendisk_partitions(struct gendisk *disk) > > Add a comment explanining the function, please. OK. > > > struct disk_part_iter piter; > > struct hd_struct *part; > > > > - blk_integrity_del(disk); > > - disk_del_events(disk); > > - > > /* invalidate stuff */ > > disk_part_iter_init(&piter, disk, > > DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE); > > @@ -714,7 +711,17 @@ void del_gendisk(struct gendisk *disk) > > disk_part_iter_exit(&piter); > > > > invalidate_partition(disk, 0); > > +} > > +EXPORT_SYMBOL(del_gendisk_partitions); > > EXPORT_SYMBOL_GPL, please. OK. -- Ming