Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1379972 > unrolled thread

Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jeff Moyer <jmoyer@redhat.com> - 2016-04-15 18:20 +0200
    Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "Verma, Vishal L" <vishal.l.verma@intel.com> - 2016-04-15 19:00 +0200
      Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jeff Moyer <jmoyer@redhat.com> - 2016-04-15 19:20 +0200
        Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "Verma, Vishal L" <vishal.l.verma@intel.com> - 2016-04-15 19:40 +0200
          Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-15 20:00 +0200
            Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jeff Moyer <jmoyer@redhat.com> - 2016-04-15 20:10 +0200
              Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-15 20:20 +0200
                Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jeff Moyer <jmoyer@redhat.com> - 2016-04-15 20:30 +0200
                  Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-15 21:00 +0200
                    Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jeff Moyer <jmoyer@redhat.com> - 2016-04-15 21:20 +0200
                Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Toshi Kani <toshi.kani@hpe.com> - 2016-04-15 21:20 +0200
                  Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Toshi Kani <toshi.kani@hpe.com> - 2016-04-15 21:40 +0200
    Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Christoph Hellwig <hch@infradead.org> - 2016-04-20 23:00 +0200
      Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "Verma, Vishal L" <vishal.l.verma@intel.com> - 2016-04-23 20:10 +0200
        Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "hch@infradead.org" <hch@infradead.org> - 2016-04-25 10:40 +0200
          Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jeff Moyer <jmoyer@redhat.com> - 2016-04-25 17:40 +0200
            Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "hch@infradead.org" <hch@infradead.org> - 2016-04-26 10:40 +0200
          Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "Verma, Vishal L" <vishal.l.verma@intel.com> - 2016-04-25 19:20 +0200
            Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-25 19:30 +0200
            Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dave Chinner <david@fromorbit.com> - 2016-04-26 01:30 +0200
              Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "Darrick J. Wong" <darrick.wong@oracle.com> - 2016-04-26 01:40 +0200
              Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-26 01:50 +0200
                Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dave Chinner <david@fromorbit.com> - 2016-04-26 02:20 +0200
                  Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-26 03:50 +0200
                    Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dave Chinner <david@fromorbit.com> - 2016-04-26 05:00 +0200
                      Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-26 06:20 +0200
                        Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dave Chinner <david@fromorbit.com> - 2016-04-26 10:30 +0200
                          Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-26 17:00 +0200
                            Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Jan Kara <jack@suse.cz> - 2016-04-26 17:40 +0200
                              Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dan Williams <dan.j.williams@intel.com> - 2016-04-26 19:20 +0200
              Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "Verma, Vishal L" <vishal.l.verma@intel.com> - 2016-04-26 02:00 +0200
                Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Dave Chinner <david@fromorbit.com> - 2016-04-26 02:50 +0200
                  Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Vishal Verma <vishal@kernel.org> - 2016-04-26 17:00 +0200
            Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io "hch@infradead.org" <hch@infradead.org> - 2016-04-26 10:40 +0200
              Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Vishal Verma <vishal@kernel.org> - 2016-04-26 17:10 +0200

Page 1 of 2  [1] 2  Next page →

#1379972 — Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io

Vishal Verma <vishal.l.verma@intel.com> writes:

> dax_do_io (called for read() or write() for a dax file system) may fail
> in the presence of bad blocks or media errors. Since we expect that a
> write should clear media errors on nvdimms, make dax_do_io fall back to
> the direct_IO path, which will send down a bio to the driver, which can
> then attempt to clear the error.

[snip]

> +	if (IS_DAX(inode)) {
> +		ret = dax_do_io(iocb, inode, iter, offset, blkdev_get_block,
>  				NULL, DIO_SKIP_DIO_COUNT);
> -	return __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter, offset,
> +		if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
> +			ret_saved = ret;
> +		else
> +			return ret;
> +	}
> +
> +	ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter, offset,
>  				    blkdev_get_block, NULL, NULL,
>  				    DIO_SKIP_DIO_COUNT);
> +	if (ret < 0 && ret_saved)
> +		return ret_saved;
> +

Hmm, did you just break async DIO?  I think you did!  :)
__blockdev_direct_IO can return -EIOCBQUEUED, and you've now turned that
into -EIO.  Really, I don't see a reason to save that first -EIO.  The
same applies to all instances in this patch.

Cheers,
Jeff


> +	return ret;
>  }
>  
>  int __sync_blockdev(struct block_device *bdev, int wait)
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 824f249..64792c6 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -859,14 +859,22 @@ ext2_direct_IO(struct kiocb *iocb, struct iov_iter *iter, loff_t offset)
>  	struct address_space *mapping = file->f_mapping;
>  	struct inode *inode = mapping->host;
>  	size_t count = iov_iter_count(iter);
> -	ssize_t ret;
> +	ssize_t ret, ret_saved = 0;
>  
> -	if (IS_DAX(inode))
> -		ret = dax_do_io(iocb, inode, iter, offset, ext2_get_block, NULL,
> -				DIO_LOCKING);
> -	else
> -		ret = blockdev_direct_IO(iocb, inode, iter, offset,
> -					 ext2_get_block);
> +	if (IS_DAX(inode)) {
> +		ret = dax_do_io(iocb, inode, iter, offset, ext2_get_block,
> +				NULL, DIO_LOCKING | DIO_SKIP_HOLES);
> +		if (ret == -EIO && iov_iter_rw(iter) == WRITE)
> +			ret_saved = ret;
> +		else
> +			goto out;
> +	}
> +
> +	ret = blockdev_direct_IO(iocb, inode, iter, offset, ext2_get_block);
> +	if (ret < 0 && ret_saved)
> +		ret = ret_saved;
> +
> + out:
>  	if (ret < 0 && iov_iter_rw(iter) == WRITE)
>  		ext2_write_failed(mapping, offset + count);
>  	return ret;
> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index 3027fa6..798f341 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -716,14 +716,22 @@ retry:
>  						   NULL, NULL, 0);
>  		inode_dio_end(inode);
>  	} else {
> +		ssize_t ret_saved = 0;
> +
>  locked:
> -		if (IS_DAX(inode))
> +		if (IS_DAX(inode)) {
>  			ret = dax_do_io(iocb, inode, iter, offset,
>  					ext4_dio_get_block, NULL, DIO_LOCKING);
> -		else
> -			ret = blockdev_direct_IO(iocb, inode, iter, offset,
> -						 ext4_dio_get_block);
> -
> +			if (ret == -EIO && iov_iter_rw(iter) == WRITE)
> +				ret_saved = ret;
> +			else
> +				goto skip_dio;
> +		}
> +		ret = blockdev_direct_IO(iocb, inode, iter, offset,
> +					 ext4_get_block);
> +		if (ret < 0 && ret_saved)
> +			ret = ret_saved;
> +skip_dio:
>  		if (unlikely(iov_iter_rw(iter) == WRITE && ret < 0)) {
>  			loff_t isize = i_size_read(inode);
>  			loff_t end = offset + count;
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index dab84a2..27f07c2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3341,7 +3341,7 @@ static ssize_t ext4_ext_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_mapping->host;
> -	ssize_t ret;
> +	ssize_t ret, ret_saved = 0;
>  	size_t count = iov_iter_count(iter);
>  	int overwrite = 0;
>  	get_block_t *get_block_func = NULL;
> @@ -3401,15 +3401,22 @@ static ssize_t ext4_ext_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  #ifdef CONFIG_EXT4_FS_ENCRYPTION
>  	BUG_ON(ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode));
>  #endif
> -	if (IS_DAX(inode))
> +	if (IS_DAX(inode)) {
>  		ret = dax_do_io(iocb, inode, iter, offset, get_block_func,
>  				ext4_end_io_dio, dio_flags);
> -	else
> -		ret = __blockdev_direct_IO(iocb, inode,
> -					   inode->i_sb->s_bdev, iter, offset,
> -					   get_block_func,
> -					   ext4_end_io_dio, NULL, dio_flags);
> +		if (ret == -EIO && iov_iter_rw(iter) == WRITE)
> +			ret_saved = ret;
> +		else
> +			goto skip_dio;
> +	}
>  
> +	ret = __blockdev_direct_IO(iocb, inode,
> +				   inode->i_sb->s_bdev, iter, offset,
> +				   get_block_func,
> +				   ext4_end_io_dio, NULL, dio_flags);
> +	if (ret < 0 && ret_saved)
> +		ret = ret_saved;
> + skip_dio:
>  	if (ret > 0 && !overwrite && ext4_test_inode_state(inode,
>  						EXT4_STATE_DIO_UNWRITTEN)) {
>  		int err;
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index d445a64..7cfcf86 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -1413,6 +1413,7 @@ xfs_vm_direct_IO(
>  	dio_iodone_t		*endio = NULL;
>  	int			flags = 0;
>  	struct block_device	*bdev;
> +	ssize_t 		ret, ret_saved = 0;
>  
>  	if (iov_iter_rw(iter) == WRITE) {
>  		endio = xfs_end_io_direct_write;
> @@ -1420,13 +1421,22 @@ xfs_vm_direct_IO(
>  	}
>  
>  	if (IS_DAX(inode)) {
> -		return dax_do_io(iocb, inode, iter, offset,
> +		ret = dax_do_io(iocb, inode, iter, offset,
>  				 xfs_get_blocks_direct, endio, 0);
> +		if (ret == -EIO && iov_iter_rw(iter) == WRITE)
> +			ret_saved = ret;
> +		else
> +			return ret;
>  	}
>  
>  	bdev = xfs_find_bdev_for_inode(inode);
> -	return  __blockdev_direct_IO(iocb, inode, bdev, iter, offset,
> +	ret = __blockdev_direct_IO(iocb, inode, bdev, iter, offset,
>  			xfs_get_blocks_direct, endio, NULL, flags);
> +
> +	if (ret < 0 && ret_saved)
> +		ret = ret_saved;
> +
> +	return ret;
>  }
>  
>  /*

[toc] | [next] | [standalone]

#1380013

On Fri, 2016-04-15 at 12:11 -0400, Jeff Moyer wrote:
> Vishal Verma <vishal.l.verma@intel.com> writes:
> 
> > 
> > dax_do_io (called for read() or write() for a dax file system) may
> > fail
> > in the presence of bad blocks or media errors. Since we expect that
> > a
> > write should clear media errors on nvdimms, make dax_do_io fall
> > back to
> > the direct_IO path, which will send down a bio to the driver, which
> > can
> > then attempt to clear the error.
> [snip]
> 
> > 
> > +	if (IS_DAX(inode)) {
> > +		ret = dax_do_io(iocb, inode, iter, offset,
> > blkdev_get_block,
> >  				NULL, DIO_SKIP_DIO_COUNT);
> > -	return __blockdev_direct_IO(iocb, inode, I_BDEV(inode),
> > iter, offset,
> > +		if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
> > +			ret_saved = ret;
> > +		else
> > +			return ret;
> > +	}
> > +
> > +	ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode),
> > iter, offset,
> >  				    blkdev_get_block, NULL, NULL,
> >  				    DIO_SKIP_DIO_COUNT);
> > +	if (ret < 0 && ret_saved)
> > +		return ret_saved;
> > +
> Hmm, did you just break async DIO?  I think you did!  :)
> __blockdev_direct_IO can return -EIOCBQUEUED, and you've now turned
> that
> into -EIO.  Really, I don't see a reason to save that first
> -EIO.  The
> same applies to all instances in this patch.

The reason I saved it was if __blockdev_direct_IO fails for some
reason, we should return the original cause o the error, which was an
EIO.. i.e. we shouldn't be hiding the EIO if the direct_IO fails with
something else..
But, how does _EIOCBQUEUED work? Maybe we need an exception for it? 

Thanks,
	-Vishal

[toc] | [prev] | [next] | [standalone]

#1380041

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Fri, 2016-04-15 at 12:11 -0400, Jeff Moyer wrote:
>> Vishal Verma <vishal.l.verma@intel.com> writes:
>> > +	if (IS_DAX(inode)) {
>> > +		ret = dax_do_io(iocb, inode, iter, offset,
>> > blkdev_get_block,
>> >  				NULL, DIO_SKIP_DIO_COUNT);
>> > -	return __blockdev_direct_IO(iocb, inode, I_BDEV(inode),
>> > iter, offset,
>> > +		if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
>> > +			ret_saved = ret;
>> > +		else
>> > +			return ret;
>> > +	}
>> > +
>> > +	ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode),
>> > iter, offset,
>> >  				    blkdev_get_block, NULL, NULL,
>> >  				    DIO_SKIP_DIO_COUNT);
>> > +	if (ret < 0 && ret_saved)
>> > +		return ret_saved;
>> > +
>> Hmm, did you just break async DIO?  I think you did!  :)
>> __blockdev_direct_IO can return -EIOCBQUEUED, and you've now turned
>> that
>> into -EIO.  Really, I don't see a reason to save that first
>> -EIO.  The
>> same applies to all instances in this patch.
>
> The reason I saved it was if __blockdev_direct_IO fails for some
> reason, we should return the original cause o the error, which was an
> EIO.. i.e. we shouldn't be hiding the EIO if the direct_IO fails with
> something else..

OK.

> But, how does _EIOCBQUEUED work? Maybe we need an exception for it?

For async direct I/O, only the setup phase of the I/O is performed and
then we return to the caller.  -EIOCBQUEUED signifies this.

You're heading towards code that looks like this:

        if (IS_DAX(inode)) {
                ret = dax_do_io(iocb, inode, iter, offset, blkdev_get_block,
                                NULL, DIO_SKIP_DIO_COUNT);
                if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
                        ret_saved = ret;
                else
                        return ret;
        }

        ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter, offset,
                                    blkdev_get_block, NULL, NULL,
                                    DIO_SKIP_DIO_COUNT);
        if (ret < 0 && ret != -EIOCBQUEUED && ret_saved)
                return ret_saved;

There's a lot of special casing here, so you might consider adding
comments.

Cheers,
Jeff

[toc] | [prev] | [next] | [standalone]

#1380061

On Fri, 2016-04-15 at 13:11 -0400, Jeff Moyer wrote:
> "Verma, Vishal L" <vishal.l.verma@intel.com> writes:
> 
> > 
> > On Fri, 2016-04-15 at 12:11 -0400, Jeff Moyer wrote:
> > > 
> > > Vishal Verma <vishal.l.verma@intel.com> writes:
> > > > 
> > > > +	if (IS_DAX(inode)) {
> > > > +		ret = dax_do_io(iocb, inode, iter, offset,
> > > > blkdev_get_block,
> > > >  				NULL, DIO_SKIP_DIO_COUNT);
> > > > -	return __blockdev_direct_IO(iocb, inode,
> > > > I_BDEV(inode),
> > > > iter, offset,
> > > > +		if (ret == -EIO && (iov_iter_rw(iter) ==
> > > > WRITE))
> > > > +			ret_saved = ret;
> > > > +		else
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode),
> > > > iter, offset,
> > > >  				    blkdev_get_block, NULL,
> > > > NULL,
> > > >  				    DIO_SKIP_DIO_COUNT);
> > > > +	if (ret < 0 && ret_saved)
> > > > +		return ret_saved;
> > > > +
> > > Hmm, did you just break async DIO?  I think you did!  :)
> > > __blockdev_direct_IO can return -EIOCBQUEUED, and you've now
> > > turned
> > > that
> > > into -EIO.  Really, I don't see a reason to save that first
> > > -EIO.  The
> > > same applies to all instances in this patch.
> > The reason I saved it was if __blockdev_direct_IO fails for some
> > reason, we should return the original cause o the error, which was
> > an
> > EIO.. i.e. we shouldn't be hiding the EIO if the direct_IO fails
> > with
> > something else..
> OK.
> 
> > 
> > But, how does _EIOCBQUEUED work? Maybe we need an exception for it?
> For async direct I/O, only the setup phase of the I/O is performed
> and
> then we return to the caller.  -EIOCBQUEUED signifies this.
> 
> You're heading towards code that looks like this:
> 
>         if (IS_DAX(inode)) {
>                 ret = dax_do_io(iocb, inode, iter, offset,
> blkdev_get_block,
>                                 NULL, DIO_SKIP_DIO_COUNT);
>                 if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
>                         ret_saved = ret;
>                 else
>                         return ret;
>         }
> 
>         ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter,
> offset,
>                                     blkdev_get_block, NULL, NULL,
>                                     DIO_SKIP_DIO_COUNT);
>         if (ret < 0 && ret != -EIOCBQUEUED && ret_saved)
>                 return ret_saved;
> 
> There's a lot of special casing here, so you might consider adding
> comments.

Correct - maybe we should reconsider wrapper-izing this? :)

Thanks for the explanation and for catching this. I'll fix it for the
next revision.

> 
> Cheers,
> Jeff

[toc] | [prev] | [next] | [standalone]

#1380073

On Fri, Apr 15, 2016 at 10:37 AM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Fri, 2016-04-15 at 13:11 -0400, Jeff Moyer wrote:
[..]
>> >
>> > But, how does _EIOCBQUEUED work? Maybe we need an exception for it?
>> For async direct I/O, only the setup phase of the I/O is performed
>> and
>> then we return to the caller.  -EIOCBQUEUED signifies this.
>>
>> You're heading towards code that looks like this:
>>
>>         if (IS_DAX(inode)) {
>>                 ret = dax_do_io(iocb, inode, iter, offset,
>> blkdev_get_block,
>>                                 NULL, DIO_SKIP_DIO_COUNT);
>>                 if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
>>                         ret_saved = ret;
>>                 else
>>                         return ret;
>>         }
>>
>>         ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter,
>> offset,
>>                                     blkdev_get_block, NULL, NULL,
>>                                     DIO_SKIP_DIO_COUNT);
>>         if (ret < 0 && ret != -EIOCBQUEUED && ret_saved)
>>                 return ret_saved;
>>
>> There's a lot of special casing here, so you might consider adding
>> comments.
>
> Correct - maybe we should reconsider wrapper-izing this? :)

Another option is just to skip dax_do_io() and this special casing
fallback entirely if errors are present.  I.e. only attempt dax_do_io
when: IS_DAX() && gendisk->bb && bb->count == 0.

[toc] | [prev] | [next] | [standalone]

#1380075

Dan Williams <dan.j.williams@intel.com> writes:

>>> There's a lot of special casing here, so you might consider adding
>>> comments.
>>
>> Correct - maybe we should reconsider wrapper-izing this? :)
>
> Another option is just to skip dax_do_io() and this special casing
> fallback entirely if errors are present.  I.e. only attempt dax_do_io
> when: IS_DAX() && gendisk->bb && bb->count == 0.

So, if there's an error anywhere on the device, penalize all I/O (not
just writes, and not just on sectors that are bad)?  I'm not sure that's
a great plan, either.

-Jeff

[toc] | [prev] | [next] | [standalone]

#1380084

On Fri, Apr 15, 2016 at 11:06 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
>
>>>> There's a lot of special casing here, so you might consider adding
>>>> comments.
>>>
>>> Correct - maybe we should reconsider wrapper-izing this? :)
>>
>> Another option is just to skip dax_do_io() and this special casing
>> fallback entirely if errors are present.  I.e. only attempt dax_do_io
>> when: IS_DAX() && gendisk->bb && bb->count == 0.
>
> So, if there's an error anywhere on the device, penalize all I/O (not
> just writes, and not just on sectors that are bad)?  I'm not sure that's
> a great plan, either.
>

If errors are rare how much are we actually losing in practice?
Moreover, we're going to do the full badblocks lookup anyway when we
call ->direct_access().  If we had that information earlier we can
avoid this fallback dance.

[toc] | [prev] | [next] | [standalone]

#1380086

Dan Williams <dan.j.williams@intel.com> writes:

> On Fri, Apr 15, 2016 at 11:06 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>>>>> There's a lot of special casing here, so you might consider adding
>>>>> comments.
>>>>
>>>> Correct - maybe we should reconsider wrapper-izing this? :)
>>>
>>> Another option is just to skip dax_do_io() and this special casing
>>> fallback entirely if errors are present.  I.e. only attempt dax_do_io
>>> when: IS_DAX() && gendisk->bb && bb->count == 0.
>>
>> So, if there's an error anywhere on the device, penalize all I/O (not
>> just writes, and not just on sectors that are bad)?  I'm not sure that's
>> a great plan, either.
>>
>
> If errors are rare how much are we actually losing in practice?

How long is a piece of string?

> Moreover, we're going to do the full badblocks lookup anyway when we
> call ->direct_access().  If we had that information earlier we can
> avoid this fallback dance.

None of the proposed approaches looks clean to me.  I'll go along with
whatever you guys think is best.  I am in favor of wrapping up all that
duplicated code, though.

Cheers,
Jeff

[toc] | [prev] | [next] | [standalone]

#1380122

On Fri, Apr 15, 2016 at 11:24 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
>> Moreover, we're going to do the full badblocks lookup anyway when we
>> call ->direct_access().  If we had that information earlier we can
>> avoid this fallback dance.
>
> None of the proposed approaches looks clean to me.  I'll go along with
> whatever you guys think is best.  I am in favor of wrapping up all that
> duplicated code, though.

Christoph originally pushed for open coding this fallback decision
per-filesystem.  I agree with you on the "none the above" options are
clean.

[toc] | [prev] | [next] | [standalone]

#1380136

Dan Williams <dan.j.williams@intel.com> writes:

> On Fri, Apr 15, 2016 at 11:24 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
>>> Moreover, we're going to do the full badblocks lookup anyway when we
>>> call ->direct_access().  If we had that information earlier we can
>>> avoid this fallback dance.
>>
>> None of the proposed approaches looks clean to me.  I'll go along with
>> whatever you guys think is best.  I am in favor of wrapping up all that
>> duplicated code, though.
>
> Christoph originally pushed for open coding this fallback decision
> per-filesystem.  I agree with you on the "none the above" options are
> clean.

I don't recall him saying "open code".  Rather, the sentiment was to
leave the fallback to the callers.  That doesn't mean you can't wrap it
up in a convenience function.

Cheers,
Jeff

[toc] | [prev] | [next] | [standalone]

#1380148

On Fri, 2016-04-15 at 11:17 -0700, Dan Williams wrote:
> On Fri, Apr 15, 2016 at 11:06 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> > 
> > Dan Williams <dan.j.williams@intel.com> writes:
> > 
> > > > > There's a lot of special casing here, so you might consider
> > > > > adding comments.
> > > > Correct - maybe we should reconsider wrapper-izing this? :)
> > > Another option is just to skip dax_do_io() and this special casing
> > > fallback entirely if errors are present.  I.e. only attempt dax_do_io
> > > when: IS_DAX() && gendisk->bb && bb->count == 0.
> >
> > So, if there's an error anywhere on the device, penalize all I/O (not
> > just writes, and not just on sectors that are bad)?  I'm not sure
> > that's a great plan, either.
> > 
> If errors are rare how much are we actually losing in practice?
> Moreover, we're going to do the full badblocks lookup anyway when we
> call ->direct_access().  If we had that information earlier we can
> avoid this fallback dance.

A system running with DAX may have active data set in NVDIMM lager than RAM
size.  In this case, falling back to non-DAX will allocate page cache for
the data, which will saturate the system with memory pressure.

Thanks,
-Toshi  

[toc] | [prev] | [next] | [standalone]

#1380180

On Fri, 2016-04-15 at 13:01 -0600, Toshi Kani wrote:
> On Fri, 2016-04-15 at 11:17 -0700, Dan Williams wrote:
> > 
> > On Fri, Apr 15, 2016 at 11:06 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> > > 
> > > Dan Williams <dan.j.williams@intel.com> writes:
> > >  
> > > > > > There's a lot of special casing here, so you might consider
> > > > > > adding comments.
> > > > > Correct - maybe we should reconsider wrapper-izing this? :)
> > > > Another option is just to skip dax_do_io() and this special casing
> > > > fallback entirely if errors are present.  I.e. only attempt
> > > > dax_do_io when: IS_DAX() && gendisk->bb && bb->count == 0.
> > >
> > > So, if there's an error anywhere on the device, penalize all I/O (not
> > > just writes, and not just on sectors that are bad)?  I'm not sure
> > > that's a great plan, either.
> > > 
> > If errors are rare how much are we actually losing in practice?
> > Moreover, we're going to do the full badblocks lookup anyway when we
> > call ->direct_access().  If we had that information earlier we can
> > avoid this fallback dance.
>
> A system running with DAX may have active data set in NVDIMM lager than
> RAM size.  In this case, falling back to non-DAX will allocate page cache
> for the data, which will saturate the system with memory pressure.

Oh, sorry, we are still in DIO path.  Falling back to DIO should not cause
this issue.

-Toshi

[toc] | [prev] | [next] | [standalone]

#1383741

On Fri, Apr 15, 2016 at 12:11:36PM -0400, Jeff Moyer wrote:
> > +	if (IS_DAX(inode)) {
> > +		ret = dax_do_io(iocb, inode, iter, offset, blkdev_get_block,
> >  				NULL, DIO_SKIP_DIO_COUNT);
> > +		if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
> > +			ret_saved = ret;
> > +		else
> > +			return ret;
> > +	}
> > +
> > +	ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter, offset,
> >  				    blkdev_get_block, NULL, NULL,
> >  				    DIO_SKIP_DIO_COUNT);
> > +	if (ret < 0 && ret_saved)
> > +		return ret_saved;
> > +
> 
> Hmm, did you just break async DIO?  I think you did!  :)
> __blockdev_direct_IO can return -EIOCBQUEUED, and you've now turned that
> into -EIO.  Really, I don't see a reason to save that first -EIO.  The
> same applies to all instances in this patch.

Yes, there is no point in saving the earlier error - just return the
second error all the time.

E.g.

	ret = dax_io();
	if (dax_need_dio_retry(ret))
		ret = direct_IO();

[toc] | [prev] | [next] | [standalone]

#1385698

On Wed, 2016-04-20 at 13:59 -0700, Christoph Hellwig wrote:
> On Fri, Apr 15, 2016 at 12:11:36PM -0400, Jeff Moyer wrote:
> > 
> > > 
> > > +	if (IS_DAX(inode)) {
> > > +		ret = dax_do_io(iocb, inode, iter, offset,
> > > blkdev_get_block,
> > >  				NULL, DIO_SKIP_DIO_COUNT);
> > > +		if (ret == -EIO && (iov_iter_rw(iter) == WRITE))
> > > +			ret_saved = ret;
> > > +		else
> > > +			return ret;
> > > +	}
> > > +
> > > +	ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode),
> > > iter, offset,
> > >  				    blkdev_get_block, NULL,
> > > NULL,
> > >  				    DIO_SKIP_DIO_COUNT);
> > > +	if (ret < 0 && ret_saved)
> > > +		return ret_saved;
> > > +
> > Hmm, did you just break async DIO?  I think you did!  :)
> > __blockdev_direct_IO can return -EIOCBQUEUED, and you've now turned
> > that
> > into -EIO.  Really, I don't see a reason to save that first
> > -EIO.  The
> > same applies to all instances in this patch.
> Yes, there is no point in saving the earlier error - just return the
> second error all the time.

Is it ok to do that?

direct_IO might fail with -EINVAL due to misalignment, or -ENOMEM due
to some allocation failing, and I thought we should return the original
-EIO in such cases so that the application doesn't lose the information
that the bad block is actually causing the error.

> 
> E.g.
> 
> 	ret = dax_io();
> 	if (dax_need_dio_retry(ret))
> 		ret = direct_IO();
> 

[toc] | [prev] | [next] | [standalone]

#1386110

On Sat, Apr 23, 2016 at 06:08:37PM +0000, Verma, Vishal L wrote:
> direct_IO might fail with -EINVAL due to misalignment, or -ENOMEM due
> to some allocation failing, and I thought we should return the original
> -EIO in such cases so that the application doesn't lose the information
> that the bad block is actually causing the error.

EINVAL is a concern here.  Not due to the right error reported, but
because it means your current scheme is fundamentally broken - we
need to support I/O at any alignment for DAX I/O, and not fail due to
alignbment concernes for a highly specific degraded case.

I think this whole series need to go back to the drawing board as I
don't think it can actually rely on using direct I/O as the EIO
fallback.

[toc] | [prev] | [next] | [standalone]

#1386600

"hch@infradead.org" <hch@infradead.org> writes:

> On Sat, Apr 23, 2016 at 06:08:37PM +0000, Verma, Vishal L wrote:
>> direct_IO might fail with -EINVAL due to misalignment, or -ENOMEM due
>> to some allocation failing, and I thought we should return the original
>> -EIO in such cases so that the application doesn't lose the information
>> that the bad block is actually causing the error.
>
> EINVAL is a concern here.  Not due to the right error reported, but
> because it means your current scheme is fundamentally broken - we
> need to support I/O at any alignment for DAX I/O, and not fail due to
> alignbment concernes for a highly specific degraded case.
>
> I think this whole series need to go back to the drawing board as I
> don't think it can actually rely on using direct I/O as the EIO
> fallback.

The only callers of dax_do_io are direct_IO methods.

Cheers,
Jeff

[toc] | [prev] | [next] | [standalone]

#1387224

On Mon, Apr 25, 2016 at 11:32:08AM -0400, Jeff Moyer wrote:
> > EINVAL is a concern here.  Not due to the right error reported, but
> > because it means your current scheme is fundamentally broken - we
> > need to support I/O at any alignment for DAX I/O, and not fail due to
> > alignbment concernes for a highly specific degraded case.
> >
> > I think this whole series need to go back to the drawing board as I
> > don't think it can actually rely on using direct I/O as the EIO
> > fallback.
> 
> The only callers of dax_do_io are direct_IO methods.

They are because the DAX I/O pass is a mess, but that doesn't mean
the user specific O_DIRECT on the open nessecarily.

[toc] | [prev] | [next] | [standalone]

#1386684

On Mon, 2016-04-25 at 01:31 -0700, hch@infradead.org wrote:
> On Sat, Apr 23, 2016 at 06:08:37PM +0000, Verma, Vishal L wrote:
> > 
> > direct_IO might fail with -EINVAL due to misalignment, or -ENOMEM
> > due
> > to some allocation failing, and I thought we should return the
> > original
> > -EIO in such cases so that the application doesn't lose the
> > information
> > that the bad block is actually causing the error.
> EINVAL is a concern here.  Not due to the right error reported, but
> because it means your current scheme is fundamentally broken - we
> need to support I/O at any alignment for DAX I/O, and not fail due to
> alignbment concernes for a highly specific degraded case.
> 
> I think this whole series need to go back to the drawing board as I
> don't think it can actually rely on using direct I/O as the EIO
> fallback.
> 
Agreed that DAX I/O can happen with any size/alignment, but how else do
we send an IO through the driver without alignment restrictions? Also,
the granularity at which we store badblocks is 512B sectors, so it
seems natural that to clear such a sector, you'd expect to send a write
to the whole sector.

The expected usage flow is:

- Application hits EIO doing dax_IO or load/store io

- It checks badblocks and discovers it's files have lost data

- It write()s those sectors (possibly converted to file offsets using
fiemap)
    * This triggers the fallback path, but if the application is doing
this level of recovery, it will know the sector is bad, and write the
entire sector

- Or it replaces the entire file from backup also using write() (not
mmap+stores)
    * This just frees the fs block, and the next time the block is
reallocated by the fs, it will likely be zeroed first, and that will be
done through the driver and will clear errors


I think if we want to keep allowing arbitrary alignments for the
dax_do_io path, we'd need:
1. To represent badblocks at a finer granularity (likely cache lines)
2. To allow the driver to do IO to a *block device* at sub-sector
granularity

Can we do that?

[toc] | [prev] | [next] | [standalone]

#1386687

On Mon, Apr 25, 2016 at 10:14 AM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Mon, 2016-04-25 at 01:31 -0700, hch@infradead.org wrote:
>> On Sat, Apr 23, 2016 at 06:08:37PM +0000, Verma, Vishal L wrote:
>> >
>> > direct_IO might fail with -EINVAL due to misalignment, or -ENOMEM
>> > due
>> > to some allocation failing, and I thought we should return the
>> > original
>> > -EIO in such cases so that the application doesn't lose the
>> > information
>> > that the bad block is actually causing the error.
>> EINVAL is a concern here.  Not due to the right error reported, but
>> because it means your current scheme is fundamentally broken - we
>> need to support I/O at any alignment for DAX I/O, and not fail due to
>> alignbment concernes for a highly specific degraded case.
>>
>> I think this whole series need to go back to the drawing board as I
>> don't think it can actually rely on using direct I/O as the EIO
>> fallback.
>>
> Agreed that DAX I/O can happen with any size/alignment, but how else do
> we send an IO through the driver without alignment restrictions? Also,
> the granularity at which we store badblocks is 512B sectors, so it
> seems natural that to clear such a sector, you'd expect to send a write
> to the whole sector.
>
> The expected usage flow is:
>
> - Application hits EIO doing dax_IO or load/store io
>
> - It checks badblocks and discovers it's files have lost data
>
> - It write()s those sectors (possibly converted to file offsets using
> fiemap)
>     * This triggers the fallback path, but if the application is doing
> this level of recovery, it will know the sector is bad, and write the
> entire sector
>
> - Or it replaces the entire file from backup also using write() (not
> mmap+stores)
>     * This just frees the fs block, and the next time the block is
> reallocated by the fs, it will likely be zeroed first, and that will be
> done through the driver and will clear errors
>
>
> I think if we want to keep allowing arbitrary alignments for the
> dax_do_io path, we'd need:
> 1. To represent badblocks at a finer granularity (likely cache lines)
> 2. To allow the driver to do IO to a *block device* at sub-sector
> granularity

3. Arrange for O_DIRECT to bypass dax_do_io(), and leave the
optimization only for the dax "buffered I/O" case.

4. Skip dax_do_io() entirely in the presence of errors

I think 3 is the most closely aligned with the typical block device
model.  In the typical case a buffered write may fail due to a
badblock read when filling the page cache, but an O_DIRECT write would
bypass the page cache and potentially clear the error / cause the
block to be reallocated internally to the drive.

[toc] | [prev] | [next] | [standalone]

#1386997

On Mon, Apr 25, 2016 at 05:14:36PM +0000, Verma, Vishal L wrote:
> On Mon, 2016-04-25 at 01:31 -0700, hch@infradead.org wrote:
> > On Sat, Apr 23, 2016 at 06:08:37PM +0000, Verma, Vishal L wrote:
> > > 
> > > direct_IO might fail with -EINVAL due to misalignment, or -ENOMEM
> > > due
> > > to some allocation failing, and I thought we should return the
> > > original
> > > -EIO in such cases so that the application doesn't lose the
> > > information
> > > that the bad block is actually causing the error.
> > EINVAL is a concern here.  Not due to the right error reported, but
> > because it means your current scheme is fundamentally broken - we
> > need to support I/O at any alignment for DAX I/O, and not fail due to
> > alignbment concernes for a highly specific degraded case.
> > 
> > I think this whole series need to go back to the drawing board as I
> > don't think it can actually rely on using direct I/O as the EIO
> > fallback.
> > 
> Agreed that DAX I/O can happen with any size/alignment, but how else do
> we send an IO through the driver without alignment restrictions? Also,
> the granularity at which we store badblocks is 512B sectors, so it
> seems natural that to clear such a sector, you'd expect to send a write
> to the whole sector.
> 
> The expected usage flow is:
> 
> - Application hits EIO doing dax_IO or load/store io
> 
> - It checks badblocks and discovers it's files have lost data

Lots of hand-waving here. How does the application map a bad
"sector" to a file without scanning the entire filesystem to find
the owner of the bad sector?

> - It write()s those sectors (possibly converted to file offsets using
> fiemap)
>     * This triggers the fallback path, but if the application is doing
> this level of recovery, it will know the sector is bad, and write the
> entire sector

Where does the application find the data that was lost to be able to
rewrite it?

> - Or it replaces the entire file from backup also using write() (not
> mmap+stores)
>     * This just frees the fs block, and the next time the block is
> reallocated by the fs, it will likely be zeroed first, and that will be
> done through the driver and will clear errors

There's an implicit assumption that applications will keep redundant
copies of their data at the /application layer/ and be able to
automatically repair it? And then there's the implicit assumption
that it will unlink and free the entire file before writing a new
copy, and that then assumes the the filesystem will zero blocks if
they get reused to clear errors on that LBA sector mapping before
they are accessible again to userspace..

It seems to me that there are a number of assumptions being made
across multiple layers here. Maybe I've missed something - can you
point me to the design/architecture description so I can see how
"app does data recovery itself" dance is supposed to work?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web