Re: [PATCH 1/1] extend BLKRRPART to update the readable size of optical media
On Fri, Feb 23, 2018 at 11:14:41AM +0100, Carlos Maiolino wrote:
> On Thu, Feb 22, 2018 at 01:00:16PM -0600, Steve Kenton wrote:
> > The readable size of new, factory blank, optical media can change via user
> > space ioctl(SG_IO) commands to format overwritable media such as DVD+RW for
> > a UDF filesystem or write to sequential media such as DVD-R for an ISO-9660
> > filesystem. However there appears to be no easy way to update the size used
> > by the block layer from the initial value of zero other than ejecting and
> > reinserting the media. This is a long standing issue which programs such as
> > growisofs work around by automatically opening and closing the tray at the
> > end of a session so the newly created filesystem becomes
> > mountable/readable. This can be problematic when using a slot load drive.
> >
This should go to linux- block btw, not linux-fsdevel, cc'ing them
> > Making the BLKRRPART ioctl call fops->revalidate_disk() =>
> > sr_revalidate_disk() in sr.c resolves the issue for optical media and
> > should be a rare and benign operation if ever called for non-optical media
> > => sd_revalidate_disk() in sd.c.
> >
>
> Please use checkpatch.pl and break the comment description to at most
> 80chars/line
>
> > Signed-off-by: Steve Kenton <sken...@ou.edu>
> > Signed-off-by: Thomas Schmitt <scdbac...@gmx.net>
> > ---
> > I discussed this with Thomas Schmitt the maintainer of the xorriso burner
> > program to define the problem and suggest a fix.
> >
> > Loading a factory blank DVD+RW needing to be "de-iced" gives the results
> > below before and after format full using xorriso but *NOT* ejecting the
> > media
> >
> > hdi@hdi-H110N:/sys/block/sr0$ dd if=/dev/sr0 of=/dev/null bs=2k
> > dd: error reading ‘/dev/sr0’: Input/output error
> > 0+0 records in
> > 0+0 records out
> > 0 bytes (0 B) copied, 0.0275589 s, 0.0 kB/s
> >
> > hdi@hdi-H110N:/sys/block/sr0$ cat size
> > 4
> >
> > hdi@hdi-H110N:/sys/block/sr0$ xorriso -dev /dev/sr0 -format full
> >
> > hdi@hdi-H110N:/sys/block/sr0$ dd if=/dev/sr0 of=/dev/null bs=2k
> > 0+0 records in
> > 0+0 records out
> > 0 bytes (0 B) copied, 0.000280081 s, 0.0 kB/s
> >
> > hdi@hdi-H110N:/sys/block/sr0$ cat size
> > 0
> >
> > Here are the results after deleting and then rescanning the optial drive
> > via sysfs
> > echo offline > state echo 1 > delete echo - - - > scan
> > *WITHOUT* ejecting the media
> >
> > hdi@hdi-H110N:/sys/block/sr0/device$ dd if=/dev/sr0 of=/dev/null bs=2k
> > count=1
> > 1+0 records in
> > 1+0 records out
> > 2048 bytes (2.0 kB) copied, 0.0624783 s, 32.8 kB/s
> >
> > hdi@hdi-H110N:/sys/block/sr0$ cat size
> > 9180416
> >
> > With this patch applied running a small program to perform ioctl(BLKRRPART)
> > on /dev/sr0 produces similar results with a blank DVD-R after writing to
> > the disc using xorriso. Again, *WITHOUT* ejecting and reinserting the
> > media. Much nicer than the spin down and delete then scan for the device
> > again business, I think :-)
> >
> > hdi@hdi-H110N:/sys/block/sr0$ cat size
> > 4
> >
> > ./blkrrpart /dev/sr0
> >
> > hdi@hdi-H110N:/sys/block/sr0$ cat size
> > 16256
> >
> >
> > block/ioctl.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/ioctl.c b/block/ioctl.c
> > index 1668506d8ed8..da79e9ba44ba 100644
> > --- a/block/ioctl.c
> > +++ b/block/ioctl.c
> > @@ -163,14 +163,18 @@ int __blkdev_reread_part(struct block_device *bdev)
> > {
> > struct gendisk *disk = bdev->bd_disk;
> >
> > - if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
> > + if (bdev != bdev->bd_contains) // must be whole disk
>
> ^^ /* Comment */
> > return -EINVAL;
> > +
> > if (!capable(CAP_SYS_ADMIN))
> > return -EACCES;
> >
> > lockdep_assert_held(>bd_mutex);
> >
> > - return rescan_partitions(disk, bdev);
> > + if (disk_part_scan_enabled(disk))
> > + return rescan_partitions(disk, bdev);
> > + else // update size but not partitions, could be sr or sd
>
> ^^ /* Comment */
> > + return disk->fops->revalidate_disk(disk);
>
> and what happens if a device without ->revalidate_disk() defined calls into
> this
> function?
>
> > }
> > EXPORT_SYMBOL(__blkdev_reread_part);
> >
> > --
> > 2.16.1.72.g5be1f00
> >
>
> --
> Carlos
--
Carlos
Re: [PATCH v7 21/22] xfs: minimal conversion to errseq_t writeback error reporting
On Fri, Jun 16, 2017 at 03:34:26PM -0400, Jeff Layton wrote:
> Just check and advance the data errseq_t in struct file before
> before returning from fsync on normal files. Internal filemap_*
> callers are left as-is.
>
Looks good.
Reviewed-by: Carlos Maiolino <cmaiol...@redhat.com>
> Signed-off-by: Jeff Layton <jlay...@redhat.com>
> ---
> fs/xfs/xfs_file.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 5fb5a0958a14..bc3b1575e8db 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -134,7 +134,7 @@ xfs_file_fsync(
> struct inode*inode = file->f_mapping->host;
> struct xfs_inode*ip = XFS_I(inode);
> struct xfs_mount*mp = ip->i_mount;
> - int error = 0;
> + int error = 0, err2;
> int log_flushed = 0;
> xfs_lsn_t lsn = 0;
>
> @@ -142,10 +142,12 @@ xfs_file_fsync(
>
> error = filemap_write_and_wait_range(inode->i_mapping, start, end);
> if (error)
> - return error;
> + goto out;
>
> - if (XFS_FORCED_SHUTDOWN(mp))
> - return -EIO;
> + if (XFS_FORCED_SHUTDOWN(mp)) {
> + error = -EIO;
> + goto out;
> + }
>
> xfs_iflags_clear(ip, XFS_ITRUNCATED);
>
> @@ -197,6 +199,11 @@ xfs_file_fsync(
> mp->m_logdev_targp == mp->m_ddev_targp)
> xfs_blkdev_issue_flush(mp->m_ddev_targp);
>
> +out:
> + err2 = filemap_report_wb_err(file);
> + if (!error)
> + error = err2;
> +
> return error;
> }
>
> --
> 2.13.0
>
--
Carlos
Re: [PATCH v7 05/22] jbd2: don't clear and reset errors after waiting on writeback
On Fri, Jun 16, 2017 at 03:34:10PM -0400, Jeff Layton wrote: > Resetting this flag is almost certainly racy, and will be problematic > with some coming changes. > > Make filemap_fdatawait_keep_errors return int, but not clear the flag(s). > Have jbd2 call it instead of filemap_fdatawait and don't attempt to > re-set the error flag if it fails. > > Signed-off-by: Jeff Layton <jlay...@redhat.com> > --- > fs/jbd2/commit.c | 15 +++ > include/linux/fs.h | 2 +- > mm/filemap.c | 16 ++-- > 3 files changed, 18 insertions(+), 15 deletions(-) > I'm not too experienced with jbd2 internals, but this patch is clear enough: Reviewed-by: Carlos Maiolino <cmaiol...@redhat.com> -- Carlos
Re: [PATCH v7 04/22] buffer: set errors in mapping at the time that the error occurs
On Fri, Jun 16, 2017 at 03:34:09PM -0400, Jeff Layton wrote:
> I noticed on xfs that I could still sometimes get back an error on fsync
> on a fd that was opened after the error condition had been cleared.
>
> The problem is that the buffer code sets the write_io_error flag and
> then later checks that flag to set the error in the mapping. That flag
> perisists for quite a while however. If the file is later opened with
> O_TRUNC, the buffers will then be invalidated and the mapping's error
> set such that a subsequent fsync will return error. I think this is
> incorrect, as there was no writeback between the open and fsync.
>
> Add a new mark_buffer_write_io_error operation that sets the flag and
> the error in the mapping at the same time. Replace all calls to
> set_buffer_write_io_error with mark_buffer_write_io_error, and remove
> the places that check this flag in order to set the error in the
> mapping.
>
> This sets the error in the mapping earlier, at the time that it's first
> detected.
>
> Signed-off-by: Jeff Layton <jlay...@redhat.com>
> Reviewed-by: Jan Kara <j...@suse.cz>
> ---
> fs/buffer.c | 20 +---
> fs/gfs2/lops.c | 2 +-
> include/linux/buffer_head.h | 1 +
> 3 files changed, 15 insertions(+), 8 deletions(-)
>
Reviewed-by: Carlos Maiolino <cmaiol...@redhat.com>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 7b4f4bfde91e..4d5d03b42e11 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -178,7 +178,7 @@ void end_buffer_write_sync(struct buffer_head *bh, int
> uptodate)
> set_buffer_uptodate(bh);
> } else {
> buffer_io_error(bh, ", lost sync page write");
> - set_buffer_write_io_error(bh);
> + mark_buffer_write_io_error(bh);
> clear_buffer_uptodate(bh);
> }
> unlock_buffer(bh);
> @@ -352,8 +352,7 @@ void end_buffer_async_write(struct buffer_head *bh, int
> uptodate)
> set_buffer_uptodate(bh);
> } else {
> buffer_io_error(bh, ", lost async page write");
> - mapping_set_error(page->mapping, -EIO);
> - set_buffer_write_io_error(bh);
> + mark_buffer_write_io_error(bh);
> clear_buffer_uptodate(bh);
> SetPageError(page);
> }
> @@ -481,8 +480,6 @@ static void __remove_assoc_queue(struct buffer_head *bh)
> {
> list_del_init(>b_assoc_buffers);
> WARN_ON(!bh->b_assoc_map);
> - if (buffer_write_io_error(bh))
> - mapping_set_error(bh->b_assoc_map, -EIO);
> bh->b_assoc_map = NULL;
> }
>
> @@ -1181,6 +1178,17 @@ void mark_buffer_dirty(struct buffer_head *bh)
> }
> EXPORT_SYMBOL(mark_buffer_dirty);
>
> +void mark_buffer_write_io_error(struct buffer_head *bh)
> +{
> + set_buffer_write_io_error(bh);
> + /* FIXME: do we need to set this in both places? */
> + if (bh->b_page && bh->b_page->mapping)
> + mapping_set_error(bh->b_page->mapping, -EIO);
> + if (bh->b_assoc_map)
> + mapping_set_error(bh->b_assoc_map, -EIO);
> +}
> +EXPORT_SYMBOL(mark_buffer_write_io_error);
> +
> /*
> * Decrement a buffer_head's reference count. If all buffers against a page
> * have zero reference count, are clean and unlocked, and if the page is
> clean
> @@ -3266,8 +3274,6 @@ drop_buffers(struct page *page, struct buffer_head
> **buffers_to_free)
>
> bh = head;
> do {
> - if (buffer_write_io_error(bh) && page->mapping)
> - mapping_set_error(page->mapping, -EIO);
> if (buffer_busy(bh))
> goto failed;
> bh = bh->b_this_page;
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 885d36e7a29f..1a9c2c08c1a1 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -182,7 +182,7 @@ static void gfs2_end_log_write_bh(struct gfs2_sbd *sdp,
> struct bio_vec *bvec,
> bh = bh->b_this_page;
> do {
> if (error)
> - set_buffer_write_io_error(bh);
> + mark_buffer_write_io_error(bh);
> unlock_buffer(bh);
> next = bh->b_this_page;
> size -= bh->b_size;
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index bd029e52ef5e..e0abeba3ced7 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -149,6 +149,7 @@ void buffer_check_dirty_writeback(struct page *page,
> */
>
> void mark_buffer_dirty(struct buffer_head *bh);
> +void mark_buffer_write_io_error(struct buffer_head *bh);
> void init_buffer(struct buffer_head *, bh_end_io_t *, void *);
> void touch_buffer(struct buffer_head *bh);
> void set_bh_page(struct buffer_head *bh,
> --
> 2.13.0
>
--
Carlos
Re: [PATCH v7 01/22] fs: remove call_fsync helper function
On Fri, Jun 16, 2017 at 03:34:06PM -0400, Jeff Layton wrote: > Requested-by: Christoph Hellwig <h...@infradead.org> > Signed-off-by: Jeff Layton <jlay...@redhat.com> > --- > fs/sync.c | 2 +- > include/linux/fs.h | 6 -- > ipc/shm.c | 2 +- > 3 files changed, 2 insertions(+), 8 deletions(-) > > 2.13.0 If it's worth to have one more reviewer, you can add: Reviewed-by: Carlos Maiolino <cmaiol...@redhat.com> > -- Carlos
