| Message ID | 20181008235823.5rpr4dqgriiacsvy@eaf (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | direct-io: allow direct writes to empty inodes | expand |
On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: > On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently > not allowed to create blocks for an empty inode. This confusion comes > from trying to bit shift a negative number, so check the size of the > inode first. > > The problem is most visible for hfsplus, because the fallback to > buffered I/O doesn't happen and the write fails with EIO. This is in > part the fault of the module, because it gives a wrong return value on > ->get_block(); that will be fixed in a separate patch. > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> Good catch. The patch looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Also Jens often picks up patches for direct IO code so added him to CC. Jens? Honza > --- > fs/direct-io.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/direct-io.c b/fs/direct-io.c > index 093fb54cd316..9a7b91a3b7a7 100644 > --- a/fs/direct-io.c > +++ b/fs/direct-io.c > @@ -679,6 +679,7 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio, > unsigned long fs_count; /* Number of filesystem-sized blocks */ > int create; > unsigned int i_blkbits = sdio->blkbits + sdio->blkfactor; > + loff_t i_size; > > /* > * If there was a memory error and we've overwritten all the > @@ -708,8 +709,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio, > */ > create = dio->op == REQ_OP_WRITE; > if (dio->flags & DIO_SKIP_HOLES) { > - if (fs_startblk <= ((i_size_read(dio->inode) - 1) >> > - i_blkbits)) > + i_size = i_size_read(dio->inode); > + if (i_size && fs_startblk <= (i_size - 1) >> i_blkbits) > create = 0; > } > > -- > 2.11.0 >
On 10/26/18 3:30 AM, Jan Kara wrote: > On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: >> On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently >> not allowed to create blocks for an empty inode. This confusion comes >> from trying to bit shift a negative number, so check the size of the >> inode first. >> >> The problem is most visible for hfsplus, because the fallback to >> buffered I/O doesn't happen and the write fails with EIO. This is in >> part the fault of the module, because it gives a wrong return value on >> ->get_block(); that will be fixed in a separate patch. >> >> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > Good catch. The patch looks good. You can add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Also Jens often picks up patches for direct IO code so added him to CC. > Jens? Looks good to me. Ernesto, did you run this through xfstests as well?
On Fri, Oct 26, 2018 at 08:29:00AM -0600, Jens Axboe wrote: > On 10/26/18 3:30 AM, Jan Kara wrote: > > On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: > >> On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently > >> not allowed to create blocks for an empty inode. This confusion comes > >> from trying to bit shift a negative number, so check the size of the > >> inode first. > >> > >> The problem is most visible for hfsplus, because the fallback to > >> buffered I/O doesn't happen and the write fails with EIO. This is in > >> part the fault of the module, because it gives a wrong return value on > >> ->get_block(); that will be fixed in a separate patch. > >> > >> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > > > Good catch. The patch looks good. You can add: > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > Also Jens often picks up patches for direct IO code so added him to CC. > > Jens? > > Looks good to me. Ernesto, did you run this through xfstests as well? I only ran the quick tests for ext2. Also for hfsplus, but those are not very meaningful because too many are failing. > > -- > Jens Axboe >
On 10/26/18 10:12 PM, Ernesto A. Fernández wrote: > On Fri, Oct 26, 2018 at 08:29:00AM -0600, Jens Axboe wrote: >> On 10/26/18 3:30 AM, Jan Kara wrote: >>> On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: >>>> On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently >>>> not allowed to create blocks for an empty inode. This confusion comes >>>> from trying to bit shift a negative number, so check the size of the >>>> inode first. >>>> >>>> The problem is most visible for hfsplus, because the fallback to >>>> buffered I/O doesn't happen and the write fails with EIO. This is in >>>> part the fault of the module, because it gives a wrong return value on >>>> ->get_block(); that will be fixed in a separate patch. >>>> >>>> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> >>> >>> Good catch. The patch looks good. You can add: >>> >>> Reviewed-by: Jan Kara <jack@suse.cz> >>> >>> Also Jens often picks up patches for direct IO code so added him to CC. >>> Jens? >> >> Looks good to me. Ernesto, did you run this through xfstests as well? > > I only ran the quick tests for ext2. Also for hfsplus, but those are not > very meaningful because too many are failing. OK, that's useful. I'll run it through the whole thing just in case, but I don't expect to find anything.
On Sat, Oct 27, 2018 at 10:54:09AM -0600, Jens Axboe wrote: > On 10/26/18 10:12 PM, Ernesto A. Fernández wrote: > > On Fri, Oct 26, 2018 at 08:29:00AM -0600, Jens Axboe wrote: > >> On 10/26/18 3:30 AM, Jan Kara wrote: > >>> On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: > >>>> On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently > >>>> not allowed to create blocks for an empty inode. This confusion comes > >>>> from trying to bit shift a negative number, so check the size of the > >>>> inode first. > >>>> > >>>> The problem is most visible for hfsplus, because the fallback to > >>>> buffered I/O doesn't happen and the write fails with EIO. This is in > >>>> part the fault of the module, because it gives a wrong return value on > >>>> ->get_block(); that will be fixed in a separate patch. > >>>> > >>>> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > >>> > >>> Good catch. The patch looks good. You can add: > >>> > >>> Reviewed-by: Jan Kara <jack@suse.cz> > >>> > >>> Also Jens often picks up patches for direct IO code so added him to CC. > >>> Jens? > >> > >> Looks good to me. Ernesto, did you run this through xfstests as well? > > > > I only ran the quick tests for ext2. Also for hfsplus, but those are not > > very meaningful because too many are failing. > > OK, that's useful. I'll run it through the whole thing just in case, > but I don't expect to find anything. So what happened with this patch? Any issues? > > -- > Jens Axboe >
On Mon, Jan 07, 2019 at 03:03:08PM -0300, Ernesto A. Fernández wrote: > On Sat, Oct 27, 2018 at 10:54:09AM -0600, Jens Axboe wrote: > > On 10/26/18 10:12 PM, Ernesto A. Fernández wrote: > > > On Fri, Oct 26, 2018 at 08:29:00AM -0600, Jens Axboe wrote: > > >> On 10/26/18 3:30 AM, Jan Kara wrote: > > >>> On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: > > >>>> On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently > > >>>> not allowed to create blocks for an empty inode. This confusion comes > > >>>> from trying to bit shift a negative number, so check the size of the > > >>>> inode first. > > >>>> > > >>>> The problem is most visible for hfsplus, because the fallback to > > >>>> buffered I/O doesn't happen and the write fails with EIO. This is in > > >>>> part the fault of the module, because it gives a wrong return value on > > >>>> ->get_block(); that will be fixed in a separate patch. > > >>>> > > >>>> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > >>> > > >>> Good catch. The patch looks good. You can add: > > >>> > > >>> Reviewed-by: Jan Kara <jack@suse.cz> > > >>> > > >>> Also Jens often picks up patches for direct IO code so added him to CC. > > >>> Jens? > > >> > > >> Looks good to me. Ernesto, did you run this through xfstests as well? > > > > > > I only ran the quick tests for ext2. Also for hfsplus, but those are not > > > very meaningful because too many are failing. > > > > OK, that's useful. I'll run it through the whole thing just in case, > > but I don't expect to find anything. > > So what happened with this patch? Any issues? Ping? > > > > > -- > > Jens Axboe > >
On 1/21/19 5:04 PM, Ernesto A. Fernández wrote: > On Mon, Jan 07, 2019 at 03:03:08PM -0300, Ernesto A. Fernández wrote: >> On Sat, Oct 27, 2018 at 10:54:09AM -0600, Jens Axboe wrote: >>> On 10/26/18 10:12 PM, Ernesto A. Fernández wrote: >>>> On Fri, Oct 26, 2018 at 08:29:00AM -0600, Jens Axboe wrote: >>>>> On 10/26/18 3:30 AM, Jan Kara wrote: >>>>>> On Mon 08-10-18 20:58:23, Ernesto A. Fernández wrote: >>>>>>> On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently >>>>>>> not allowed to create blocks for an empty inode. This confusion comes >>>>>>> from trying to bit shift a negative number, so check the size of the >>>>>>> inode first. >>>>>>> >>>>>>> The problem is most visible for hfsplus, because the fallback to >>>>>>> buffered I/O doesn't happen and the write fails with EIO. This is in >>>>>>> part the fault of the module, because it gives a wrong return value on >>>>>>> ->get_block(); that will be fixed in a separate patch. >>>>>>> >>>>>>> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> >>>>>> >>>>>> Good catch. The patch looks good. You can add: >>>>>> >>>>>> Reviewed-by: Jan Kara <jack@suse.cz> >>>>>> >>>>>> Also Jens often picks up patches for direct IO code so added him to CC. >>>>>> Jens? >>>>> >>>>> Looks good to me. Ernesto, did you run this through xfstests as well? >>>> >>>> I only ran the quick tests for ext2. Also for hfsplus, but those are not >>>> very meaningful because too many are failing. >>> >>> OK, that's useful. I'll run it through the whole thing just in case, >>> but I don't expect to find anything. >> >> So what happened with this patch? Any issues? > > Ping? Sorry, I dropped this one on the floor. Now queued up, thanks for the ping!
diff --git a/fs/direct-io.c b/fs/direct-io.c index 093fb54cd316..9a7b91a3b7a7 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -679,6 +679,7 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio, unsigned long fs_count; /* Number of filesystem-sized blocks */ int create; unsigned int i_blkbits = sdio->blkbits + sdio->blkfactor; + loff_t i_size; /* * If there was a memory error and we've overwritten all the @@ -708,8 +709,8 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio, */ create = dio->op == REQ_OP_WRITE; if (dio->flags & DIO_SKIP_HOLES) { - if (fs_startblk <= ((i_size_read(dio->inode) - 1) >> - i_blkbits)) + i_size = i_size_read(dio->inode); + if (i_size && fs_startblk <= (i_size - 1) >> i_blkbits) create = 0; }
On a DIO_SKIP_HOLES filesystem, the ->get_block() method is currently not allowed to create blocks for an empty inode. This confusion comes from trying to bit shift a negative number, so check the size of the inode first. The problem is most visible for hfsplus, because the fallback to buffered I/O doesn't happen and the write fails with EIO. This is in part the fault of the module, because it gives a wrong return value on ->get_block(); that will be fixed in a separate patch. Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> --- fs/direct-io.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)