Changwei Ge
2017-Dec-19 05:50 UTC
[Ocfs2-devel] [PATCH] ocfs2: fall back to buffer IO when append dio is disabled with file hole existing
On 2017/12/19 10:35, Gang He wrote:> Hi Changwei, > > >>>> >> Before ocfs2 supporting allocating clusters while doing append-dio, all append >> dio will fall back to buffer io to allocate clusters firstly. Also, when it >> steps on a file hole, it will fall back to buffer io, too. But for current >> code, writing to file hole will leverage dio to allocate clusters. This is >> not >> right, since whether append-io is enabled tells the capability whether ocfs2 >> can >> allocate space while doing dio. >> So introduce file hole check function back into ocfs2. >> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will >> fall >> back to buffer IO to allocate clusters. >> >> Signed-off-by: Changwei Ge <ge.changwei at h3c.com> >> --- >> fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 42 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >> index d151632..a982cf6 100644 >> --- a/fs/ocfs2/aops.c >> +++ b/fs/ocfs2/aops.c >> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb, >> return ret; >> } >> >> +/* >> + * Will look for holes and unwritten extents in the range starting at >> + * pos for count bytes (inclusive). >> + */ > Why do we also need to look for unwritten extents here? unwritten extents means, the clusters have been allocated, but have not been written yet. > Unwritten extents will not bring any block allocation, my understanding is right here? >Hi Gang, For now, I think your comment here makes scene. Unwritten cluster should not make dio fall back to buffer io. Actually, I copied this function snippet from earlier version of ocfs2 :)>> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos, >> + size_t count) >> +{ >> + int ret = 0; >> + unsigned int extent_flags; >> + u32 cpos, clusters, extent_len, phys_cpos; >> + struct super_block *sb = inode->i_sb; >> + >> + cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits; >> + clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos; > Please consider data-inline case, if in data-inline case, we maybe need not to allocate more cluster. > Then, if there is not any more block need to be allocated, we should make the check return true.I suppose current code in ocfs2_direct_IO already checks this for us. Thanks, Changwei> >> + >> + while (clusters) { >> + ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len, >> + &extent_flags); >> + if (ret < 0) { >> + mlog_errno(ret); >> + goto out; >> + } >> + >> + if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) { >> + ret = 1; >> + break; >> + } >> + >> + if (extent_len > clusters) >> + extent_len = clusters; >> + >> + clusters -= extent_len; >> + cpos += extent_len; >> + } >> +out: >> + return ret; >> +} >> + >> static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) >> { >> struct file *file = iocb->ki_filp; >> @@ -2429,8 +2467,10 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, >> struct iov_iter *iter) >> return 0; >> >> /* Fallback to buffered I/O if we do not support append dio. */ >> - if (iocb->ki_pos + iter->count > i_size_read(inode) && >> - !ocfs2_supports_append_dio(osb)) >> + if (!ocfs2_supports_append_dio(osb) && >> + (iocb->ki_pos + iter->count > i_size_read(inode) || >> + ocfs2_check_range_for_holes(inode, iocb->ki_pos, >> + iter->count))) >> return 0; >> >> if (iov_iter_rw(iter) == READ) >> -- >> 2.7.4 >> >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel at oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel > >
Gang He
2017-Dec-19 06:27 UTC
[Ocfs2-devel] [PATCH] ocfs2: fall back to buffer IO when append dio is disabled with file hole existing
>>> > On 2017/12/19 10:35, Gang He wrote: >> Hi Changwei, >> >> >>>>> >>> Before ocfs2 supporting allocating clusters while doing append-dio, all > append >>> dio will fall back to buffer io to allocate clusters firstly. Also, when it >>> steps on a file hole, it will fall back to buffer io, too. But for current >>> code, writing to file hole will leverage dio to allocate clusters. This is >>> not >>> right, since whether append-io is enabled tells the capability whether ocfs2 >>> can >>> allocate space while doing dio. >>> So introduce file hole check function back into ocfs2. >>> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will >>> fall >>> back to buffer IO to allocate clusters. >>> >>> Signed-off-by: Changwei Ge <ge.changwei at h3c.com> >>> --- >>> fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 42 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >>> index d151632..a982cf6 100644 >>> --- a/fs/ocfs2/aops.c >>> +++ b/fs/ocfs2/aops.c >>> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb, >>> return ret; >>> } >>> >>> +/* >>> + * Will look for holes and unwritten extents in the range starting at >>> + * pos for count bytes (inclusive). >>> + */ >> Why do we also need to look for unwritten extents here? unwritten extents > means, the clusters have been allocated, but have not been written yet. >> Unwritten extents will not bring any block allocation, my understanding is > right here? >> > > Hi Gang, > For now, I think your comment here makes scene. > Unwritten cluster should not make dio fall back to buffer io. > Actually, I copied this function snippet from earlier version of ocfs2 :) > >>> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos, >>> + size_t count) >>> +{ >>> + int ret = 0; >>> + unsigned int extent_flags; >>> + u32 cpos, clusters, extent_len, phys_cpos; >>> + struct super_block *sb = inode->i_sb; >>> + >>> + cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits; >>> + clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos; >> Please consider data-inline case, if in data-inline case, we maybe need not to > allocate more cluster. >> Then, if there is not any more block need to be allocated, we should make > the check return true. > > I suppose current code in ocfs2_direct_IO already checks this for us.Hi Changwei, please take a look again. My means, if the user try to do di-write a inline-data inode, the inode has not any cluster, but still can write in inline-data area. like the similar cases, maybe you need to check, make sure all the cases can work well. Thanks Gang> > Thanks, > Changwei > >> >>> + >>> + while (clusters) { >>> + ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len, >>> + &extent_flags); >>> + if (ret < 0) { >>> + mlog_errno(ret); >>> + goto out; >>> + } >>> + >>> + if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) { >>> + ret = 1; >>> + break; >>> + } >>> + >>> + if (extent_len > clusters) >>> + extent_len = clusters; >>> + >>> + clusters -= extent_len; >>> + cpos += extent_len; >>> + } >>> +out: >>> + return ret; >>> +} >>> + >>> static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) >>> { >>> struct file *file = iocb->ki_filp; >>> @@ -2429,8 +2467,10 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, >>> struct iov_iter *iter) >>> return 0; >>> >>> /* Fallback to buffered I/O if we do not support append dio. */ >>> - if (iocb->ki_pos + iter->count > i_size_read(inode) && >>> - !ocfs2_supports_append_dio(osb)) >>> + if (!ocfs2_supports_append_dio(osb) && >>> + (iocb->ki_pos + iter->count > i_size_read(inode) || >>> + ocfs2_check_range_for_holes(inode, iocb->ki_pos, >>> + iter->count))) >>> return 0; >>> >>> if (iov_iter_rw(iter) == READ) >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> Ocfs2-devel mailing list >>> Ocfs2-devel at oss.oracle.com >>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >> >>