All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Hannes Reinecke <hare@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Mike Snitzer <snitzer@redhat.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-block@vger.kernel.org, dm-devel@redhat.com,
	Linux Next Mailing List <linux-next@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>,
	Alasdair G Kergon <agk@redhat.com>
Subject: Re: [dm-devel] DM's filesystem lookup in dm_get_dev_t() [was: Re: linux-next: manual merge of the device-mapper tree with Linus' tree]
Date: Wed, 23 Dec 2020 09:05:37 +0100	[thread overview]
Message-ID: <20201223080537.GA4974@lst.de> (raw)
In-Reply-To: <288d1c58-c0e2-9d6f-4816-48c66536fe8b@suse.de>

On Tue, Dec 22, 2020 at 06:24:09PM +0100, Hannes Reinecke wrote:
> Ok. The problem from my perspective is that device-mapper needs to
> a) ensure that the arbitrary string passed in with the table definition 
> refers to a valid block device
> and
> b) the block device can be opened with O_EXCL, so that device-mapper can 
> then use it.
>
> Originally (ie prior to commit 644bda6f3460) dm_get_device() just converted 
> the string to a 'dev_t' representation, and then the block device itself 
> was checked and opened in dm_get_table_device().
> 'lookup_bdev' was just being used to convert the path if the string was not 
> in the canonical major:minor format, as then it was assumed that it 
> referred to a block device node, and then lookup_bdev kinda makes sense.

Yes, 644bda6f3460 is the cause of all evil, as it uses an API in a place
where it should not be used.  It and the prep patch
(e6e20a7a5f3f49bfee518d5c6849107398d83912) which did the grave mistake
of making name_to_dev_t available outside of the early init code really
need to be reverted.

> However, lookup_bdev() now always recurses into the filesystem, causing 
> multipath to stall in an all-paths-down scenario.

lookup_bdev always did a file system lookup, and always only accepted
a valid name in the file system.

> Alternatively, if Mike says that only major:minor is the valid format for a 
> table definition we can kill that code completely. But clearly _I_ cannot 
> make the call here.

Before 644bda6f3460 the table definitions only accepted a valid name in
the file system.  Which is the proper interface.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Hannes Reinecke <hare@suse.de>
Cc: Mike Snitzer <snitzer@redhat.com>, Christoph Hellwig <hch@lst.de>,
	linux-block@vger.kernel.org, dm-devel@redhat.com,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Alasdair G Kergon <agk@redhat.com>, Jens Axboe <axboe@kernel.dk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux Next Mailing List <linux-next@vger.kernel.org>
Subject: Re: DM's filesystem lookup in dm_get_dev_t() [was: Re: linux-next: manual merge of the device-mapper tree with Linus' tree]
Date: Wed, 23 Dec 2020 09:05:37 +0100	[thread overview]
Message-ID: <20201223080537.GA4974@lst.de> (raw)
In-Reply-To: <288d1c58-c0e2-9d6f-4816-48c66536fe8b@suse.de>

On Tue, Dec 22, 2020 at 06:24:09PM +0100, Hannes Reinecke wrote:
> Ok. The problem from my perspective is that device-mapper needs to
> a) ensure that the arbitrary string passed in with the table definition 
> refers to a valid block device
> and
> b) the block device can be opened with O_EXCL, so that device-mapper can 
> then use it.
>
> Originally (ie prior to commit 644bda6f3460) dm_get_device() just converted 
> the string to a 'dev_t' representation, and then the block device itself 
> was checked and opened in dm_get_table_device().
> 'lookup_bdev' was just being used to convert the path if the string was not 
> in the canonical major:minor format, as then it was assumed that it 
> referred to a block device node, and then lookup_bdev kinda makes sense.

Yes, 644bda6f3460 is the cause of all evil, as it uses an API in a place
where it should not be used.  It and the prep patch
(e6e20a7a5f3f49bfee518d5c6849107398d83912) which did the grave mistake
of making name_to_dev_t available outside of the early init code really
need to be reverted.

> However, lookup_bdev() now always recurses into the filesystem, causing 
> multipath to stall in an all-paths-down scenario.

lookup_bdev always did a file system lookup, and always only accepted
a valid name in the file system.

> Alternatively, if Mike says that only major:minor is the valid format for a 
> table definition we can kill that code completely. But clearly _I_ cannot 
> make the call here.

Before 644bda6f3460 the table definitions only accepted a valid name in
the file system.  Which is the proper interface.

  parent reply	other threads:[~2020-12-23  8:06 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-21 22:50 linux-next: manual merge of the device-mapper tree with Linus' tree Stephen Rothwell
2020-12-22 13:15 ` Christoph Hellwig
2020-12-22 14:53   ` [dm-devel] DM's filesystem lookup in dm_get_dev_t() [was: Re: linux-next: manual merge of the device-mapper tree with Linus' tree] Mike Snitzer
2020-12-22 14:53     ` Mike Snitzer
2020-12-22 17:24     ` [dm-devel] " Hannes Reinecke
2020-12-22 17:24       ` Hannes Reinecke
2020-12-22 21:06       ` [dm-devel] " Alasdair G Kergon
2020-12-22 21:06         ` Alasdair G Kergon
2020-12-23  8:06         ` Christoph Hellwig
2020-12-23  8:06           ` Christoph Hellwig
2020-12-23  8:05       ` Christoph Hellwig [this message]
2020-12-23  8:05         ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201223080537.GA4974@lst.de \
    --to=hch@lst.de \
    --cc=agk@redhat.com \
    --cc=axboe@kernel.dk \
    --cc=dm-devel@redhat.com \
    --cc=hare@suse.de \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-next@vger.kernel.org \
    --cc=sfr@canb.auug.org.au \
    --cc=snitzer@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.