[RFC,1/2] block: allow other bd i_node flags when DAX is disabled
diff mbox

Message ID 1463074986-3070-2-git-send-email-jonathan.derrick@intel.com
State New
Headers show

Commit Message

Derrick, Jonathan May 12, 2016, 5:43 p.m. UTC
When DAX is not compiled into the kernel or the device does not support
direct-access, the block device file's inode flags are fully cleared.
This patch changes it to only clear the S_DAX flag when DAX is disabled.

This reverts to i_flags behavior prior to
bbab37ddc20bae4709bca8745c128c4f46fe63c5

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 fs/block_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dan Williams May 13, 2016, 1:25 p.m. UTC | #1
On Thu, May 12, 2016 at 10:43 AM, Jon Derrick
<jonathan.derrick@intel.com> wrote:
> When DAX is not compiled into the kernel or the device does not support
> direct-access, the block device file's inode flags are fully cleared.
> This patch changes it to only clear the S_DAX flag when DAX is disabled.
>
> This reverts to i_flags behavior prior to
> bbab37ddc20bae4709bca8745c128c4f46fe63c5
>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  fs/block_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 20a2c02..d4fa725 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1208,7 +1208,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>                 if (IS_ENABLED(CONFIG_BLK_DEV_DAX) && disk->fops->direct_access)
>                         bdev->bd_inode->i_flags = S_DAX;
>                 else
> -                       bdev->bd_inode->i_flags = 0;
> +                       bdev->bd_inode->i_flags &= ~S_DAX;

Setting S_DAX is atomic, but the above change makes it a non-atomic
read-modify-write.  Do we need exclusion / locking in this path?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Moyer May 13, 2016, 5:33 p.m. UTC | #2
Dan Williams <dan.j.williams@intel.com> writes:

> On Thu, May 12, 2016 at 10:43 AM, Jon Derrick
> <jonathan.derrick@intel.com> wrote:
>> When DAX is not compiled into the kernel or the device does not support
>> direct-access, the block device file's inode flags are fully cleared.
>> This patch changes it to only clear the S_DAX flag when DAX is disabled.
>>
>> This reverts to i_flags behavior prior to
>> bbab37ddc20bae4709bca8745c128c4f46fe63c5
>>
>> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
>> ---
>>  fs/block_dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index 20a2c02..d4fa725 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -1208,7 +1208,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>>                 if (IS_ENABLED(CONFIG_BLK_DEV_DAX) && disk->fops->direct_access)
>>                         bdev->bd_inode->i_flags = S_DAX;
>>                 else
>> -                       bdev->bd_inode->i_flags = 0;
>> +                       bdev->bd_inode->i_flags &= ~S_DAX;
>
> Setting S_DAX is atomic, but the above change makes it a non-atomic
> read-modify-write.  Do we need exclusion / locking in this path?

First, I don't see how you'd ever get a read-modify-write here, as S_DAX
surely won't ever be set if direct_access isn't supported.  Second,
there is existing code that already does this very thing.  See further
down in __blkdev_get:

                        if (!ret) {
                                bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
                                if (!blkdev_dax_capable(bdev))
                                        bdev->bd_inode->i_flags &= ~S_DAX;
                        }

What relies on S_DAX being set or cleared atomically?

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams May 13, 2016, 5:53 p.m. UTC | #3
On Fri, May 13, 2016 at 10:33 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
>
>> On Thu, May 12, 2016 at 10:43 AM, Jon Derrick
>> <jonathan.derrick@intel.com> wrote:
>>> When DAX is not compiled into the kernel or the device does not support
>>> direct-access, the block device file's inode flags are fully cleared.
>>> This patch changes it to only clear the S_DAX flag when DAX is disabled.
>>>
>>> This reverts to i_flags behavior prior to
>>> bbab37ddc20bae4709bca8745c128c4f46fe63c5
>>>
>>> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
>>> ---
>>>  fs/block_dev.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>>> index 20a2c02..d4fa725 100644
>>> --- a/fs/block_dev.c
>>> +++ b/fs/block_dev.c
>>> @@ -1208,7 +1208,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>>>                 if (IS_ENABLED(CONFIG_BLK_DEV_DAX) && disk->fops->direct_access)
>>>                         bdev->bd_inode->i_flags = S_DAX;
>>>                 else
>>> -                       bdev->bd_inode->i_flags = 0;
>>> +                       bdev->bd_inode->i_flags &= ~S_DAX;
>>
>> Setting S_DAX is atomic, but the above change makes it a non-atomic
>> read-modify-write.  Do we need exclusion / locking in this path?
>
> First, I don't see how you'd ever get a read-modify-write here, as S_DAX
> surely won't ever be set if direct_access isn't supported.  Second,
> there is existing code that already does this very thing.  See further
> down in __blkdev_get:
>
>                         if (!ret) {
>                                 bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
>                                 if (!blkdev_dax_capable(bdev))
>                                         bdev->bd_inode->i_flags &= ~S_DAX;
>                         }
>
> What relies on S_DAX being set or cleared atomically?

So, when I wrote the above "&= ~S_DAX" I was worried about the
non-atomic update with respect to the BLKDAXSET ioctl, but we killed
BLKDAXSET so the worry went away.  Now an inode flag setting ioctl is
back for the S_HIPRI case.  Can that collide with these other flag
touches?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Derrick, Jonathan May 13, 2016, 6:01 p.m. UTC | #4
On Fri, May 13, 2016 at 10:53:37AM -0700, Dan Williams wrote:
> On Fri, May 13, 2016 at 10:33 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> > Dan Williams <dan.j.williams@intel.com> writes:
> >
> >> On Thu, May 12, 2016 at 10:43 AM, Jon Derrick
> >> <jonathan.derrick@intel.com> wrote:
> >>> When DAX is not compiled into the kernel or the device does not support
> >>> direct-access, the block device file's inode flags are fully cleared.
> >>> This patch changes it to only clear the S_DAX flag when DAX is disabled.
> >>>
> >>> This reverts to i_flags behavior prior to
> >>> bbab37ddc20bae4709bca8745c128c4f46fe63c5
> >>>
> >>> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> >>> ---
> >>>  fs/block_dev.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/block_dev.c b/fs/block_dev.c
> >>> index 20a2c02..d4fa725 100644
> >>> --- a/fs/block_dev.c
> >>> +++ b/fs/block_dev.c
> >>> @@ -1208,7 +1208,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
> >>>                 if (IS_ENABLED(CONFIG_BLK_DEV_DAX) && disk->fops->direct_access)
> >>>                         bdev->bd_inode->i_flags = S_DAX;
> >>>                 else
> >>> -                       bdev->bd_inode->i_flags = 0;
> >>> +                       bdev->bd_inode->i_flags &= ~S_DAX;
> >>
> >> Setting S_DAX is atomic, but the above change makes it a non-atomic
> >> read-modify-write.  Do we need exclusion / locking in this path?
> >
> > First, I don't see how you'd ever get a read-modify-write here, as S_DAX
> > surely won't ever be set if direct_access isn't supported.  Second,
> > there is existing code that already does this very thing.  See further
> > down in __blkdev_get:
> >
> >                         if (!ret) {
> >                                 bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
> >                                 if (!blkdev_dax_capable(bdev))
> >                                         bdev->bd_inode->i_flags &= ~S_DAX;
> >                         }
> >
> > What relies on S_DAX being set or cleared atomically?
> 
> So, when I wrote the above "&= ~S_DAX" I was worried about the
> non-atomic update with respect to the BLKDAXSET ioctl, but we killed
> BLKDAXSET so the worry went away.  Now an inode flag setting ioctl is
> back for the S_HIPRI case.  Can that collide with these other flag
> touches?

The S_DAX case is unclear to me, but the new ioctl certainly does need a i_mutex lock and to use inode_set_flags.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 20a2c02..d4fa725 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1208,7 +1208,7 @@  static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 		if (IS_ENABLED(CONFIG_BLK_DEV_DAX) && disk->fops->direct_access)
 			bdev->bd_inode->i_flags = S_DAX;
 		else
-			bdev->bd_inode->i_flags = 0;
+			bdev->bd_inode->i_flags &= ~S_DAX;
 
 		if (!partno) {
 			ret = -ENXIO;