* SEEK_DATA/HOLE on ocfs2 - v2 @ 2011-05-19 2:44 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 2:44 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, o Two patches follow this message. One fixes the default implementation of SEEK_HOLE/DATA. This patch applies atop Josef's last posted patch. The second patch implements the same on ocfs2. The test tool for the same is available here. http://oss.oracle.com/~smushran/seek_data/seek_test.c It is improved since the last post. It runs cleanly on zfs, ocfs2 and ext3 (default behavior). Users testing on zfs will need to flip the values of SEEK_HOLE/DATA. The results on ext4 and btrfs are also available here. http://oss.oracle.com/~smushran/seek_data/ Thanks Sunil ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Ocfs2-devel] SEEK_DATA/HOLE on ocfs2 - v2 @ 2011-05-19 2:44 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 2:44 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro Two patches follow this message. One fixes the default implementation of SEEK_HOLE/DATA. This patch applies atop Josef's last posted patch. The second patch implements the same on ocfs2. The test tool for the same is available here. http://oss.oracle.com/~smushran/seek_data/seek_test.c It is improved since the last post. It runs cleanly on zfs, ocfs2 and ext3 (default behavior). Users testing on zfs will need to flip the values of SEEK_HOLE/DATA. The results on ext4 and btrfs are also available here. http://oss.oracle.com/~smushran/seek_data/ Thanks Sunil ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] fs: Fix default SEEK_DATA and SEEK_HOLE implementation 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran @ 2011-05-19 2:44 ` Sunil Mushran -1 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 2:44 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, o In the default SEEK_DATA implementation, as the entire file is treated as data, all supplied offsets less than the size of the file are valid. For empty files, SEEK_DATA and SEEK_HOLE returns -ENXIO irrespective of the input offset. Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> --- fs/read_write.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/fs/read_write.c b/fs/read_write.c index af9cc51..4b991e9 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -66,11 +66,10 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin) break; case SEEK_DATA: /* - * In the generic case the entire file is data, so data only - * starts at position 0 provided the file has an i_size, - * otherwise it's an empty file and will always be ENXIO. + * In the generic case the entire file is data. So all offsets + * less than the file size are valid. */ - if (offset != 0 || inode->i_size == 0) + if (inode->i_size == 0 || offset >= inode->i_size) return -ENXIO; break; case SEEK_HOLE: @@ -78,7 +77,7 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin) * There is a virtual hole at the end of the file, so as long as * offset isn't i_size or larger, return i_size. */ - if (offset >= inode->i_size) + if (inode->i_size == 0 || offset >= inode->i_size) return -ENXIO; offset = inode->i_size; break; -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Ocfs2-devel] [PATCH] fs: Fix default SEEK_DATA and SEEK_HOLE implementation @ 2011-05-19 2:44 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 2:44 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro In the default SEEK_DATA implementation, as the entire file is treated as data, all supplied offsets less than the size of the file are valid. For empty files, SEEK_DATA and SEEK_HOLE returns -ENXIO irrespective of the input offset. Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> --- fs/read_write.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/fs/read_write.c b/fs/read_write.c index af9cc51..4b991e9 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -66,11 +66,10 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin) break; case SEEK_DATA: /* - * In the generic case the entire file is data, so data only - * starts at position 0 provided the file has an i_size, - * otherwise it's an empty file and will always be ENXIO. + * In the generic case the entire file is data. So all offsets + * less than the file size are valid. */ - if (offset != 0 || inode->i_size == 0) + if (inode->i_size == 0 || offset >= inode->i_size) return -ENXIO; break; case SEEK_HOLE: @@ -78,7 +77,7 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin) * There is a virtual hole at the end of the file, so as long as * offset isn't i_size or larger, return i_size. */ - if (offset >= inode->i_size) + if (inode->i_size == 0 || offset >= inode->i_size) return -ENXIO; offset = inode->i_size; break; -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] ocfs2: Implement llseek() 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran @ 2011-05-19 2:44 ` Sunil Mushran -1 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 2:44 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, o ocfs2 implements its own llseek() to provide the SEEK_HOLE/SEEK_DATA functionality. SEEK_HOLE sets the file pointer to the start of either a hole or an unwritten (preallocated) extent, that is greater than or equal to the supplied offset. SEEK_DATA sets the file pointer to the start of an allocated extent (not unwritten) that is greater than or equal to the supplied offset. If the supplied offset is on a desired region, then the file pointer is set to it. Offsets greater than or equal to the file size return -ENXIO. Unwritten (preallocated) extents are considered holes because the file system treats reads to such regions in the same way as it does to holes. Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> --- fs/ocfs2/extent_map.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ fs/ocfs2/extent_map.h | 2 + fs/ocfs2/file.c | 53 ++++++++++++++++++++++++++- 3 files changed, 150 insertions(+), 2 deletions(-) diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c index 23457b4..6942c21 100644 --- a/fs/ocfs2/extent_map.c +++ b/fs/ocfs2/extent_map.c @@ -832,6 +832,103 @@ out: return ret; } +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret; + unsigned int is_last = 0, is_data = 0; + u16 cs_bits = OCFS2_SB(inode->i_sb)->s_clustersize_bits; + u32 cpos, cend, clen, hole_size; + u64 extoff, extlen; + struct buffer_head *di_bh = NULL; + struct ocfs2_extent_rec rec; + + BUG_ON(origin != SEEK_DATA && origin != SEEK_HOLE); + + ret = ocfs2_inode_lock(inode, &di_bh, 0); + if (ret) { + mlog_errno(ret); + goto out; + } + + down_read(&OCFS2_I(inode)->ip_alloc_sem); + + if (inode->i_size == 0 || *offset >= inode->i_size) { + ret = -ENXIO; + goto out_unlock; + } + + if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { + if (origin == SEEK_HOLE) + *offset = inode->i_size; + goto out_unlock; + } + + clen = 0; + cpos = *offset >> cs_bits; + cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); + + while (cpos < cend && !is_last) { + ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, + &rec, &is_last); + if (ret) { + mlog_errno(ret); + goto out_unlock; + } + + extoff = cpos; + extoff <<= cs_bits; + + if (rec.e_blkno == 0ULL) { + clen = hole_size; + is_data = 0; + } else { + BUG_ON(cpos < le32_to_cpu(rec.e_cpos)); + clen = le16_to_cpu(rec.e_leaf_clusters) - + (cpos - le32_to_cpu(rec.e_cpos)); + is_data = (rec.e_flags & OCFS2_EXT_UNWRITTEN) ? 0 : 1; + } + + if ((!is_data && origin == SEEK_HOLE) || + (is_data && origin == SEEK_DATA)) { + if (extoff > *offset) + *offset = extoff; + goto out_unlock; + } + + if (!is_last) + cpos += clen; + } + + if (origin == SEEK_HOLE) { + extoff = cpos; + extoff <<= cs_bits; + extlen = clen; + extlen <<= cs_bits; + + if ((extoff + extlen) > inode->i_size) + extlen = inode->i_size - extoff; + extoff += extlen; + if (extoff > *offset) + *offset = extoff; + goto out_unlock; + } + + ret = -ENXIO; + +out_unlock: + + brelse(di_bh); + + up_read(&OCFS2_I(inode)->ip_alloc_sem); + + ocfs2_inode_unlock(inode, 0); +out: + if (ret && ret != -ENXIO) + ret = -ENXIO; + return ret; +} + int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr, struct buffer_head *bhs[], int flags, int (*validate)(struct super_block *sb, diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h index e79d41c..67ea57d 100644 --- a/fs/ocfs2/extent_map.h +++ b/fs/ocfs2/extent_map.h @@ -53,6 +53,8 @@ int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 map_start, u64 map_len); +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin); + int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster, u32 *p_cluster, u32 *num_clusters, struct ocfs2_extent_list *el, diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index cce8c2b..5349a4b 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -2572,6 +2572,55 @@ bail: return ret; } +/* Refer generic_file_llseek_unlocked() */ +static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret = 0; + + mutex_lock(&inode->i_mutex); + + switch (origin) { + case SEEK_END: + offset += inode->i_size; + break; + case SEEK_CUR: + if (offset == 0) { + offset = file->f_pos; + goto out; + } + offset += file->f_pos; + break; + case SEEK_DATA: + case SEEK_HOLE: + ret = ocfs2_seek_data_hole_offset(file, &offset, origin); + if (ret) + goto out; + break; + default: + ret = -EINVAL; + goto out; + } + + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) + ret = -EINVAL; + if (!ret && offset > inode->i_sb->s_maxbytes) + ret = -EINVAL; + if (ret) + goto out; + + if (offset != file->f_pos) { + file->f_pos = offset; + file->f_version = 0; + } + +out: + mutex_unlock(&inode->i_mutex); + if (ret) + return ret; + return offset; +} + const struct inode_operations ocfs2_file_iops = { .setattr = ocfs2_setattr, .getattr = ocfs2_getattr, @@ -2594,7 +2643,7 @@ const struct inode_operations ocfs2_special_file_iops = { * ocfs2_fops_no_plocks and ocfs2_dops_no_plocks! */ const struct file_operations ocfs2_fops = { - .llseek = generic_file_llseek, + .llseek = ocfs2_file_llseek, .read = do_sync_read, .write = do_sync_write, .mmap = ocfs2_mmap, @@ -2642,7 +2691,7 @@ const struct file_operations ocfs2_dops = { * the cluster. */ const struct file_operations ocfs2_fops_no_plocks = { - .llseek = generic_file_llseek, + .llseek = ocfs2_file_llseek, .read = do_sync_read, .write = do_sync_write, .mmap = ocfs2_mmap, -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() @ 2011-05-19 2:44 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 2:44 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro ocfs2 implements its own llseek() to provide the SEEK_HOLE/SEEK_DATA functionality. SEEK_HOLE sets the file pointer to the start of either a hole or an unwritten (preallocated) extent, that is greater than or equal to the supplied offset. SEEK_DATA sets the file pointer to the start of an allocated extent (not unwritten) that is greater than or equal to the supplied offset. If the supplied offset is on a desired region, then the file pointer is set to it. Offsets greater than or equal to the file size return -ENXIO. Unwritten (preallocated) extents are considered holes because the file system treats reads to such regions in the same way as it does to holes. Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> --- fs/ocfs2/extent_map.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ fs/ocfs2/extent_map.h | 2 + fs/ocfs2/file.c | 53 ++++++++++++++++++++++++++- 3 files changed, 150 insertions(+), 2 deletions(-) diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c index 23457b4..6942c21 100644 --- a/fs/ocfs2/extent_map.c +++ b/fs/ocfs2/extent_map.c @@ -832,6 +832,103 @@ out: return ret; } +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret; + unsigned int is_last = 0, is_data = 0; + u16 cs_bits = OCFS2_SB(inode->i_sb)->s_clustersize_bits; + u32 cpos, cend, clen, hole_size; + u64 extoff, extlen; + struct buffer_head *di_bh = NULL; + struct ocfs2_extent_rec rec; + + BUG_ON(origin != SEEK_DATA && origin != SEEK_HOLE); + + ret = ocfs2_inode_lock(inode, &di_bh, 0); + if (ret) { + mlog_errno(ret); + goto out; + } + + down_read(&OCFS2_I(inode)->ip_alloc_sem); + + if (inode->i_size == 0 || *offset >= inode->i_size) { + ret = -ENXIO; + goto out_unlock; + } + + if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { + if (origin == SEEK_HOLE) + *offset = inode->i_size; + goto out_unlock; + } + + clen = 0; + cpos = *offset >> cs_bits; + cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); + + while (cpos < cend && !is_last) { + ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, + &rec, &is_last); + if (ret) { + mlog_errno(ret); + goto out_unlock; + } + + extoff = cpos; + extoff <<= cs_bits; + + if (rec.e_blkno == 0ULL) { + clen = hole_size; + is_data = 0; + } else { + BUG_ON(cpos < le32_to_cpu(rec.e_cpos)); + clen = le16_to_cpu(rec.e_leaf_clusters) - + (cpos - le32_to_cpu(rec.e_cpos)); + is_data = (rec.e_flags & OCFS2_EXT_UNWRITTEN) ? 0 : 1; + } + + if ((!is_data && origin == SEEK_HOLE) || + (is_data && origin == SEEK_DATA)) { + if (extoff > *offset) + *offset = extoff; + goto out_unlock; + } + + if (!is_last) + cpos += clen; + } + + if (origin == SEEK_HOLE) { + extoff = cpos; + extoff <<= cs_bits; + extlen = clen; + extlen <<= cs_bits; + + if ((extoff + extlen) > inode->i_size) + extlen = inode->i_size - extoff; + extoff += extlen; + if (extoff > *offset) + *offset = extoff; + goto out_unlock; + } + + ret = -ENXIO; + +out_unlock: + + brelse(di_bh); + + up_read(&OCFS2_I(inode)->ip_alloc_sem); + + ocfs2_inode_unlock(inode, 0); +out: + if (ret && ret != -ENXIO) + ret = -ENXIO; + return ret; +} + int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr, struct buffer_head *bhs[], int flags, int (*validate)(struct super_block *sb, diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h index e79d41c..67ea57d 100644 --- a/fs/ocfs2/extent_map.h +++ b/fs/ocfs2/extent_map.h @@ -53,6 +53,8 @@ int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 map_start, u64 map_len); +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin); + int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster, u32 *p_cluster, u32 *num_clusters, struct ocfs2_extent_list *el, diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index cce8c2b..5349a4b 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -2572,6 +2572,55 @@ bail: return ret; } +/* Refer generic_file_llseek_unlocked() */ +static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret = 0; + + mutex_lock(&inode->i_mutex); + + switch (origin) { + case SEEK_END: + offset += inode->i_size; + break; + case SEEK_CUR: + if (offset == 0) { + offset = file->f_pos; + goto out; + } + offset += file->f_pos; + break; + case SEEK_DATA: + case SEEK_HOLE: + ret = ocfs2_seek_data_hole_offset(file, &offset, origin); + if (ret) + goto out; + break; + default: + ret = -EINVAL; + goto out; + } + + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) + ret = -EINVAL; + if (!ret && offset > inode->i_sb->s_maxbytes) + ret = -EINVAL; + if (ret) + goto out; + + if (offset != file->f_pos) { + file->f_pos = offset; + file->f_version = 0; + } + +out: + mutex_unlock(&inode->i_mutex); + if (ret) + return ret; + return offset; +} + const struct inode_operations ocfs2_file_iops = { .setattr = ocfs2_setattr, .getattr = ocfs2_getattr, @@ -2594,7 +2643,7 @@ const struct inode_operations ocfs2_special_file_iops = { * ocfs2_fops_no_plocks and ocfs2_dops_no_plocks! */ const struct file_operations ocfs2_fops = { - .llseek = generic_file_llseek, + .llseek = ocfs2_file_llseek, .read = do_sync_read, .write = do_sync_write, .mmap = ocfs2_mmap, @@ -2642,7 +2691,7 @@ const struct file_operations ocfs2_dops = { * the cluster. */ const struct file_operations ocfs2_fops_no_plocks = { - .llseek = generic_file_llseek, + .llseek = ocfs2_file_llseek, .read = do_sync_read, .write = do_sync_write, .mmap = ocfs2_mmap, -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] ocfs2: Implement llseek() 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran (?) @ 2011-05-19 9:13 ` Tristan Ye -1 siblings, 0 replies; 18+ messages in thread From: Tristan Ye @ 2011-05-19 9:13 UTC (permalink / raw) To: Sunil Mushran Cc: linux-kernel, ocfs2-devel, josef, linux-fsdevel, linux-ext4, linux-btrfs, viro Sunil Mushran wrote: > ocfs2 implements its own llseek() to provide the SEEK_HOLE/SEEK_DATA > functionality. > > SEEK_HOLE sets the file pointer to the start of either a hole or an unwritten > (preallocated) extent, that is greater than or equal to the supplied offset. > > SEEK_DATA sets the file pointer to the start of an allocated extent (not > unwritten) that is greater than or equal to the supplied offset. > > If the supplied offset is on a desired region, then the file pointer is set > to it. Offsets greater than or equal to the file size return -ENXIO. > > Unwritten (preallocated) extents are considered holes because the file system > treats reads to such regions in the same way as it does to holes. > > Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> > --- > fs/ocfs2/extent_map.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ > fs/ocfs2/extent_map.h | 2 + > fs/ocfs2/file.c | 53 ++++++++++++++++++++++++++- > 3 files changed, 150 insertions(+), 2 deletions(-) > > diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c > index 23457b4..6942c21 100644 > --- a/fs/ocfs2/extent_map.c > +++ b/fs/ocfs2/extent_map.c > @@ -832,6 +832,103 @@ out: > return ret; > } > > +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin) > +{ > + struct inode *inode = file->f_mapping->host; > + int ret; > + unsigned int is_last = 0, is_data = 0; > + u16 cs_bits = OCFS2_SB(inode->i_sb)->s_clustersize_bits; > + u32 cpos, cend, clen, hole_size; > + u64 extoff, extlen; > + struct buffer_head *di_bh = NULL; > + struct ocfs2_extent_rec rec; > + > + BUG_ON(origin != SEEK_DATA && origin != SEEK_HOLE); > + > + ret = ocfs2_inode_lock(inode, &di_bh, 0); > + if (ret) { > + mlog_errno(ret); > + goto out; > + } > + > + down_read(&OCFS2_I(inode)->ip_alloc_sem); > + > + if (inode->i_size == 0 || *offset >= inode->i_size) { > + ret = -ENXIO; > + goto out_unlock; > + } Why not using if (*offset >= inode->i_size) directly? > + > + if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { > + if (origin == SEEK_HOLE) > + *offset = inode->i_size; > + goto out_unlock; > + } > + > + clen = 0; > + cpos = *offset >> cs_bits; > + cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); > + > + while (cpos < cend && !is_last) { > + ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, > + &rec, &is_last); > + if (ret) { > + mlog_errno(ret); > + goto out_unlock; > + } > + > + extoff = cpos; > + extoff <<= cs_bits; > + > + if (rec.e_blkno == 0ULL) { > + clen = hole_size; > + is_data = 0; > + } else { > + BUG_ON(cpos < le32_to_cpu(rec.e_cpos)); A same assert has already been performed inside ocfs2_get_clusters_nocache(), does it make sense to do it again here? > + clen = le16_to_cpu(rec.e_leaf_clusters) - > + (cpos - le32_to_cpu(rec.e_cpos)); > + is_data = (rec.e_flags & OCFS2_EXT_UNWRITTEN) ? 0 : 1; > + } > + > + if ((!is_data && origin == SEEK_HOLE) || > + (is_data && origin == SEEK_DATA)) { > + if (extoff > *offset) > + *offset = extoff; > + goto out_unlock; Seems above logic is going to stop at the first time we find a hole. How about the offset was within the range of a hole already when we doing SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose start_offset was greater than supplied offset, according to semantics described by the the header of this patch, should it be like following? if (extoff > *offset) { *offset = extoff; goto out_unlock; } > + } > + > + if (!is_last) > + cpos += clen; > + } > + > + if (origin == SEEK_HOLE) { > + extoff = cpos; > + extoff <<= cs_bits; extoff already has been assigned properly above in while loop? > + extlen = clen; > + extlen <<= cs_bits; > + > + if ((extoff + extlen) > inode->i_size) > + extlen = inode->i_size - extoff; > + extoff += extlen; > + if (extoff > *offset) > + *offset = extoff; > + goto out_unlock; > + } > + > + ret = -ENXIO; > + > +out_unlock: > + > + brelse(di_bh); > + > + up_read(&OCFS2_I(inode)->ip_alloc_sem); > + > + ocfs2_inode_unlock(inode, 0); > +out: > + if (ret && ret != -ENXIO) > + ret = -ENXIO; > + return ret; > +} > + > int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr, > struct buffer_head *bhs[], int flags, > int (*validate)(struct super_block *sb, > diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h > index e79d41c..67ea57d 100644 > --- a/fs/ocfs2/extent_map.h > +++ b/fs/ocfs2/extent_map.h > @@ -53,6 +53,8 @@ int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, > int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > u64 map_start, u64 map_len); > > +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin); > + > int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster, > u32 *p_cluster, u32 *num_clusters, > struct ocfs2_extent_list *el, > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c > index cce8c2b..5349a4b 100644 > --- a/fs/ocfs2/file.c > +++ b/fs/ocfs2/file.c > @@ -2572,6 +2572,55 @@ bail: > return ret; > } > > +/* Refer generic_file_llseek_unlocked() */ > +static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int origin) > +{ > + struct inode *inode = file->f_mapping->host; > + int ret = 0; > + > + mutex_lock(&inode->i_mutex); > + > + switch (origin) { > + case SEEK_END: > + offset += inode->i_size; > + break; > + case SEEK_CUR: > + if (offset == 0) { > + offset = file->f_pos; > + goto out; > + } > + offset += file->f_pos; > + break; > + case SEEK_DATA: > + case SEEK_HOLE: > + ret = ocfs2_seek_data_hole_offset(file, &offset, origin); > + if (ret) > + goto out; > + break; > + default: > + ret = -EINVAL; > + goto out; > + } > + > + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) > + ret = -EINVAL; > + if (!ret && offset > inode->i_sb->s_maxbytes) > + ret = -EINVAL; > + if (ret) > + goto out; > + > + if (offset != file->f_pos) { > + file->f_pos = offset; > + file->f_version = 0; > + } > + > +out: > + mutex_unlock(&inode->i_mutex); > + if (ret) > + return ret; > + return offset; > +} > + > const struct inode_operations ocfs2_file_iops = { > .setattr = ocfs2_setattr, > .getattr = ocfs2_getattr, > @@ -2594,7 +2643,7 @@ const struct inode_operations ocfs2_special_file_iops = { > * ocfs2_fops_no_plocks and ocfs2_dops_no_plocks! > */ > const struct file_operations ocfs2_fops = { > - .llseek = generic_file_llseek, > + .llseek = ocfs2_file_llseek, > .read = do_sync_read, > .write = do_sync_write, > .mmap = ocfs2_mmap, > @@ -2642,7 +2691,7 @@ const struct file_operations ocfs2_dops = { > * the cluster. > */ > const struct file_operations ocfs2_fops_no_plocks = { > - .llseek = generic_file_llseek, > + .llseek = ocfs2_file_llseek, > .read = do_sync_read, > .write = do_sync_write, > .mmap = ocfs2_mmap, ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() @ 2011-05-19 9:13 ` Tristan Ye 0 siblings, 0 replies; 18+ messages in thread From: Tristan Ye @ 2011-05-19 9:13 UTC (permalink / raw) To: Sunil Mushran Cc: linux-kernel, ocfs2-devel, josef, linux-fsdevel, linux-ext4, linux-btrfs, viro Sunil Mushran wrote: > ocfs2 implements its own llseek() to provide the SEEK_HOLE/SEEK_DATA > functionality. > > SEEK_HOLE sets the file pointer to the start of either a hole or an unwritten > (preallocated) extent, that is greater than or equal to the supplied offset. > > SEEK_DATA sets the file pointer to the start of an allocated extent (not > unwritten) that is greater than or equal to the supplied offset. > > If the supplied offset is on a desired region, then the file pointer is set > to it. Offsets greater than or equal to the file size return -ENXIO. > > Unwritten (preallocated) extents are considered holes because the file system > treats reads to such regions in the same way as it does to holes. > > Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> > --- > fs/ocfs2/extent_map.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ > fs/ocfs2/extent_map.h | 2 + > fs/ocfs2/file.c | 53 ++++++++++++++++++++++++++- > 3 files changed, 150 insertions(+), 2 deletions(-) > > diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c > index 23457b4..6942c21 100644 > --- a/fs/ocfs2/extent_map.c > +++ b/fs/ocfs2/extent_map.c > @@ -832,6 +832,103 @@ out: > return ret; > } > > +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin) > +{ > + struct inode *inode = file->f_mapping->host; > + int ret; > + unsigned int is_last = 0, is_data = 0; > + u16 cs_bits = OCFS2_SB(inode->i_sb)->s_clustersize_bits; > + u32 cpos, cend, clen, hole_size; > + u64 extoff, extlen; > + struct buffer_head *di_bh = NULL; > + struct ocfs2_extent_rec rec; > + > + BUG_ON(origin != SEEK_DATA && origin != SEEK_HOLE); > + > + ret = ocfs2_inode_lock(inode, &di_bh, 0); > + if (ret) { > + mlog_errno(ret); > + goto out; > + } > + > + down_read(&OCFS2_I(inode)->ip_alloc_sem); > + > + if (inode->i_size == 0 || *offset >= inode->i_size) { > + ret = -ENXIO; > + goto out_unlock; > + } Why not using if (*offset >= inode->i_size) directly? > + > + if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { > + if (origin == SEEK_HOLE) > + *offset = inode->i_size; > + goto out_unlock; > + } > + > + clen = 0; > + cpos = *offset >> cs_bits; > + cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); > + > + while (cpos < cend && !is_last) { > + ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, > + &rec, &is_last); > + if (ret) { > + mlog_errno(ret); > + goto out_unlock; > + } > + > + extoff = cpos; > + extoff <<= cs_bits; > + > + if (rec.e_blkno == 0ULL) { > + clen = hole_size; > + is_data = 0; > + } else { > + BUG_ON(cpos < le32_to_cpu(rec.e_cpos)); A same assert has already been performed inside ocfs2_get_clusters_nocache(), does it make sense to do it again here? > + clen = le16_to_cpu(rec.e_leaf_clusters) - > + (cpos - le32_to_cpu(rec.e_cpos)); > + is_data = (rec.e_flags & OCFS2_EXT_UNWRITTEN) ? 0 : 1; > + } > + > + if ((!is_data && origin == SEEK_HOLE) || > + (is_data && origin == SEEK_DATA)) { > + if (extoff > *offset) > + *offset = extoff; > + goto out_unlock; Seems above logic is going to stop at the first time we find a hole. How about the offset was within the range of a hole already when we doing SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose start_offset was greater than supplied offset, according to semantics described by the the header of this patch, should it be like following? if (extoff > *offset) { *offset = extoff; goto out_unlock; } > + } > + > + if (!is_last) > + cpos += clen; > + } > + > + if (origin == SEEK_HOLE) { > + extoff = cpos; > + extoff <<= cs_bits; extoff already has been assigned properly above in while loop? > + extlen = clen; > + extlen <<= cs_bits; > + > + if ((extoff + extlen) > inode->i_size) > + extlen = inode->i_size - extoff; > + extoff += extlen; > + if (extoff > *offset) > + *offset = extoff; > + goto out_unlock; > + } > + > + ret = -ENXIO; > + > +out_unlock: > + > + brelse(di_bh); > + > + up_read(&OCFS2_I(inode)->ip_alloc_sem); > + > + ocfs2_inode_unlock(inode, 0); > +out: > + if (ret && ret != -ENXIO) > + ret = -ENXIO; > + return ret; > +} > + > int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr, > struct buffer_head *bhs[], int flags, > int (*validate)(struct super_block *sb, > diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h > index e79d41c..67ea57d 100644 > --- a/fs/ocfs2/extent_map.h > +++ b/fs/ocfs2/extent_map.h > @@ -53,6 +53,8 @@ int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, > int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > u64 map_start, u64 map_len); > > +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin); > + > int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster, > u32 *p_cluster, u32 *num_clusters, > struct ocfs2_extent_list *el, > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c > index cce8c2b..5349a4b 100644 > --- a/fs/ocfs2/file.c > +++ b/fs/ocfs2/file.c > @@ -2572,6 +2572,55 @@ bail: > return ret; > } > > +/* Refer generic_file_llseek_unlocked() */ > +static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int origin) > +{ > + struct inode *inode = file->f_mapping->host; > + int ret = 0; > + > + mutex_lock(&inode->i_mutex); > + > + switch (origin) { > + case SEEK_END: > + offset += inode->i_size; > + break; > + case SEEK_CUR: > + if (offset == 0) { > + offset = file->f_pos; > + goto out; > + } > + offset += file->f_pos; > + break; > + case SEEK_DATA: > + case SEEK_HOLE: > + ret = ocfs2_seek_data_hole_offset(file, &offset, origin); > + if (ret) > + goto out; > + break; > + default: > + ret = -EINVAL; > + goto out; > + } > + > + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) > + ret = -EINVAL; > + if (!ret && offset > inode->i_sb->s_maxbytes) > + ret = -EINVAL; > + if (ret) > + goto out; > + > + if (offset != file->f_pos) { > + file->f_pos = offset; > + file->f_version = 0; > + } > + > +out: > + mutex_unlock(&inode->i_mutex); > + if (ret) > + return ret; > + return offset; > +} > + > const struct inode_operations ocfs2_file_iops = { > .setattr = ocfs2_setattr, > .getattr = ocfs2_getattr, > @@ -2594,7 +2643,7 @@ const struct inode_operations ocfs2_special_file_iops = { > * ocfs2_fops_no_plocks and ocfs2_dops_no_plocks! > */ > const struct file_operations ocfs2_fops = { > - .llseek = generic_file_llseek, > + .llseek = ocfs2_file_llseek, > .read = do_sync_read, > .write = do_sync_write, > .mmap = ocfs2_mmap, > @@ -2642,7 +2691,7 @@ const struct file_operations ocfs2_dops = { > * the cluster. > */ > const struct file_operations ocfs2_fops_no_plocks = { > - .llseek = generic_file_llseek, > + .llseek = ocfs2_file_llseek, > .read = do_sync_read, > .write = do_sync_write, > .mmap = ocfs2_mmap, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() @ 2011-05-19 9:13 ` Tristan Ye 0 siblings, 0 replies; 18+ messages in thread From: Tristan Ye @ 2011-05-19 9:13 UTC (permalink / raw) To: Sunil Mushran Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel Sunil Mushran wrote: > ocfs2 implements its own llseek() to provide the SEEK_HOLE/SEEK_DATA > functionality. > > SEEK_HOLE sets the file pointer to the start of either a hole or an unwritten > (preallocated) extent, that is greater than or equal to the supplied offset. > > SEEK_DATA sets the file pointer to the start of an allocated extent (not > unwritten) that is greater than or equal to the supplied offset. > > If the supplied offset is on a desired region, then the file pointer is set > to it. Offsets greater than or equal to the file size return -ENXIO. > > Unwritten (preallocated) extents are considered holes because the file system > treats reads to such regions in the same way as it does to holes. > > Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> > --- > fs/ocfs2/extent_map.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ > fs/ocfs2/extent_map.h | 2 + > fs/ocfs2/file.c | 53 ++++++++++++++++++++++++++- > 3 files changed, 150 insertions(+), 2 deletions(-) > > diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c > index 23457b4..6942c21 100644 > --- a/fs/ocfs2/extent_map.c > +++ b/fs/ocfs2/extent_map.c > @@ -832,6 +832,103 @@ out: > return ret; > } > > +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin) > +{ > + struct inode *inode = file->f_mapping->host; > + int ret; > + unsigned int is_last = 0, is_data = 0; > + u16 cs_bits = OCFS2_SB(inode->i_sb)->s_clustersize_bits; > + u32 cpos, cend, clen, hole_size; > + u64 extoff, extlen; > + struct buffer_head *di_bh = NULL; > + struct ocfs2_extent_rec rec; > + > + BUG_ON(origin != SEEK_DATA && origin != SEEK_HOLE); > + > + ret = ocfs2_inode_lock(inode, &di_bh, 0); > + if (ret) { > + mlog_errno(ret); > + goto out; > + } > + > + down_read(&OCFS2_I(inode)->ip_alloc_sem); > + > + if (inode->i_size == 0 || *offset >= inode->i_size) { > + ret = -ENXIO; > + goto out_unlock; > + } Why not using if (*offset >= inode->i_size) directly? > + > + if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { > + if (origin == SEEK_HOLE) > + *offset = inode->i_size; > + goto out_unlock; > + } > + > + clen = 0; > + cpos = *offset >> cs_bits; > + cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); > + > + while (cpos < cend && !is_last) { > + ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, > + &rec, &is_last); > + if (ret) { > + mlog_errno(ret); > + goto out_unlock; > + } > + > + extoff = cpos; > + extoff <<= cs_bits; > + > + if (rec.e_blkno == 0ULL) { > + clen = hole_size; > + is_data = 0; > + } else { > + BUG_ON(cpos < le32_to_cpu(rec.e_cpos)); A same assert has already been performed inside ocfs2_get_clusters_nocache(), does it make sense to do it again here? > + clen = le16_to_cpu(rec.e_leaf_clusters) - > + (cpos - le32_to_cpu(rec.e_cpos)); > + is_data = (rec.e_flags & OCFS2_EXT_UNWRITTEN) ? 0 : 1; > + } > + > + if ((!is_data && origin == SEEK_HOLE) || > + (is_data && origin == SEEK_DATA)) { > + if (extoff > *offset) > + *offset = extoff; > + goto out_unlock; Seems above logic is going to stop at the first time we find a hole. How about the offset was within the range of a hole already when we doing SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose start_offset was greater than supplied offset, according to semantics described by the the header of this patch, should it be like following? if (extoff > *offset) { *offset = extoff; goto out_unlock; } > + } > + > + if (!is_last) > + cpos += clen; > + } > + > + if (origin == SEEK_HOLE) { > + extoff = cpos; > + extoff <<= cs_bits; extoff already has been assigned properly above in while loop? > + extlen = clen; > + extlen <<= cs_bits; > + > + if ((extoff + extlen) > inode->i_size) > + extlen = inode->i_size - extoff; > + extoff += extlen; > + if (extoff > *offset) > + *offset = extoff; > + goto out_unlock; > + } > + > + ret = -ENXIO; > + > +out_unlock: > + > + brelse(di_bh); > + > + up_read(&OCFS2_I(inode)->ip_alloc_sem); > + > + ocfs2_inode_unlock(inode, 0); > +out: > + if (ret && ret != -ENXIO) > + ret = -ENXIO; > + return ret; > +} > + > int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr, > struct buffer_head *bhs[], int flags, > int (*validate)(struct super_block *sb, > diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h > index e79d41c..67ea57d 100644 > --- a/fs/ocfs2/extent_map.h > +++ b/fs/ocfs2/extent_map.h > @@ -53,6 +53,8 @@ int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, > int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > u64 map_start, u64 map_len); > > +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin); > + > int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster, > u32 *p_cluster, u32 *num_clusters, > struct ocfs2_extent_list *el, > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c > index cce8c2b..5349a4b 100644 > --- a/fs/ocfs2/file.c > +++ b/fs/ocfs2/file.c > @@ -2572,6 +2572,55 @@ bail: > return ret; > } > > +/* Refer generic_file_llseek_unlocked() */ > +static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int origin) > +{ > + struct inode *inode = file->f_mapping->host; > + int ret = 0; > + > + mutex_lock(&inode->i_mutex); > + > + switch (origin) { > + case SEEK_END: > + offset += inode->i_size; > + break; > + case SEEK_CUR: > + if (offset == 0) { > + offset = file->f_pos; > + goto out; > + } > + offset += file->f_pos; > + break; > + case SEEK_DATA: > + case SEEK_HOLE: > + ret = ocfs2_seek_data_hole_offset(file, &offset, origin); > + if (ret) > + goto out; > + break; > + default: > + ret = -EINVAL; > + goto out; > + } > + > + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) > + ret = -EINVAL; > + if (!ret && offset > inode->i_sb->s_maxbytes) > + ret = -EINVAL; > + if (ret) > + goto out; > + > + if (offset != file->f_pos) { > + file->f_pos = offset; > + file->f_version = 0; > + } > + > +out: > + mutex_unlock(&inode->i_mutex); > + if (ret) > + return ret; > + return offset; > +} > + > const struct inode_operations ocfs2_file_iops = { > .setattr = ocfs2_setattr, > .getattr = ocfs2_getattr, > @@ -2594,7 +2643,7 @@ const struct inode_operations ocfs2_special_file_iops = { > * ocfs2_fops_no_plocks and ocfs2_dops_no_plocks! > */ > const struct file_operations ocfs2_fops = { > - .llseek = generic_file_llseek, > + .llseek = ocfs2_file_llseek, > .read = do_sync_read, > .write = do_sync_write, > .mmap = ocfs2_mmap, > @@ -2642,7 +2691,7 @@ const struct file_operations ocfs2_dops = { > * the cluster. > */ > const struct file_operations ocfs2_fops_no_plocks = { > - .llseek = generic_file_llseek, > + .llseek = ocfs2_file_llseek, > .read = do_sync_read, > .write = do_sync_write, > .mmap = ocfs2_mmap, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() 2011-05-19 9:13 ` Tristan Ye @ 2011-05-19 17:45 ` Sunil Mushran -1 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 17:45 UTC (permalink / raw) To: Tristan Ye Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On 05/19/2011 02:13 AM, Tristan Ye wrote: >> + if (inode->i_size == 0 || *offset>= inode->i_size) { >> + ret = -ENXIO; >> + goto out_unlock; >> + } > Why not using if (*offset>= inode->i_size) directly? duh! > + BUG_ON(cpos< le32_to_cpu(rec.e_cpos)); > A same assert has already been performed inside ocfs2_get_clusters_nocache(), > does it make sense to do it again here? good catch >> + >> + if ((!is_data&& origin == SEEK_HOLE) || >> + (is_data&& origin == SEEK_DATA)) { >> + if (extoff> *offset) >> + *offset = extoff; >> + goto out_unlock; > Seems above logic is going to stop at the first time we find a hole. > > How about the offset was within the range of a hole already when we doing > SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose > start_offset was greater than supplied offset, according to semantics described > by the the header of this patch, should it be like following? > > if (extoff> *offset) { > *offset = extoff; > goto out_unlock; > } So if the offset is in a hole, then we set the file pointer to it. Same for data. The file pointer is set to the region asked at an offset that is equal to or greater than the supplied offset. >> + if (origin == SEEK_HOLE) { >> + extoff = cpos; >> + extoff<<= cs_bits; > extoff already has been assigned properly above in while loop? To handle the case when supplied cpos == cend. As always, excellent review. Thanks Sunil ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() @ 2011-05-19 17:45 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 17:45 UTC (permalink / raw) To: Tristan Ye Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On 05/19/2011 02:13 AM, Tristan Ye wrote: >> + if (inode->i_size == 0 || *offset>= inode->i_size) { >> + ret = -ENXIO; >> + goto out_unlock; >> + } > Why not using if (*offset>= inode->i_size) directly? duh! > + BUG_ON(cpos< le32_to_cpu(rec.e_cpos)); > A same assert has already been performed inside ocfs2_get_clusters_nocache(), > does it make sense to do it again here? good catch >> + >> + if ((!is_data&& origin == SEEK_HOLE) || >> + (is_data&& origin == SEEK_DATA)) { >> + if (extoff> *offset) >> + *offset = extoff; >> + goto out_unlock; > Seems above logic is going to stop at the first time we find a hole. > > How about the offset was within the range of a hole already when we doing > SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose > start_offset was greater than supplied offset, according to semantics described > by the the header of this patch, should it be like following? > > if (extoff> *offset) { > *offset = extoff; > goto out_unlock; > } So if the offset is in a hole, then we set the file pointer to it. Same for data. The file pointer is set to the region asked at an offset that is equal to or greater than the supplied offset. >> + if (origin == SEEK_HOLE) { >> + extoff = cpos; >> + extoff<<= cs_bits; > extoff already has been assigned properly above in while loop? To handle the case when supplied cpos == cend. As always, excellent review. Thanks Sunil ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran @ 2011-05-19 11:05 ` Christoph Hellwig -1 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2011-05-19 11:05 UTC (permalink / raw) To: Sunil Mushran Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On Wed, May 18, 2011 at 07:44:44PM -0700, Sunil Mushran wrote: > Unwritten (preallocated) extents are considered holes because the file system > treats reads to such regions in the same way as it does to holes. How does this work for the case of an unwrittent extent that has been written to in the pagecache but not converted yet? Y'know the big data corruption and flamewar that started all this? ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() @ 2011-05-19 11:05 ` Christoph Hellwig 0 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2011-05-19 11:05 UTC (permalink / raw) To: Sunil Mushran Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On Wed, May 18, 2011 at 07:44:44PM -0700, Sunil Mushran wrote: > Unwritten (preallocated) extents are considered holes because the file system > treats reads to such regions in the same way as it does to holes. How does this work for the case of an unwrittent extent that has been written to in the pagecache but not converted yet? Y'know the big data corruption and flamewar that started all this? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() 2011-05-19 11:05 ` Christoph Hellwig @ 2011-05-19 17:29 ` Sunil Mushran -1 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 17:29 UTC (permalink / raw) To: Christoph Hellwig Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On 05/19/2011 04:05 AM, Christoph Hellwig wrote: > On Wed, May 18, 2011 at 07:44:44PM -0700, Sunil Mushran wrote: >> Unwritten (preallocated) extents are considered holes because the file system >> treats reads to such regions in the same way as it does to holes. > How does this work for the case of an unwrittent extent that has been > written to in the pagecache but not converted yet? Y'know the big data > corruption and flamewar that started all this? We don't delay splitting the extent. It is split in ->write_begin(). Delaying it will be a challenge as we have to provide cache coherency across the cluster. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() @ 2011-05-19 17:29 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-19 17:29 UTC (permalink / raw) To: Christoph Hellwig Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On 05/19/2011 04:05 AM, Christoph Hellwig wrote: > On Wed, May 18, 2011 at 07:44:44PM -0700, Sunil Mushran wrote: >> Unwritten (preallocated) extents are considered holes because the file system >> treats reads to such regions in the same way as it does to holes. > How does this work for the case of an unwrittent extent that has been > written to in the pagecache but not converted yet? Y'know the big data > corruption and flamewar that started all this? We don't delay splitting the extent. It is split in ->write_begin(). Delaying it will be a challenge as we have to provide cache coherency across the cluster. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Ocfs2-devel] SEEK_DATA/HOLE on ocfs2 - v2 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran @ 2011-05-19 11:04 ` Christoph Hellwig -1 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2011-05-19 11:04 UTC (permalink / raw) To: Sunil Mushran Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On Wed, May 18, 2011 at 07:44:42PM -0700, Sunil Mushran wrote: > It is improved since the last post. It runs cleanly on zfs, ocfs2 and ext3 > (default behavior). Users testing on zfs will need to flip the values of > SEEK_HOLE/DATA. sounds like we should switch the around, just to cause the least amount of confusion. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Ocfs2-devel] SEEK_DATA/HOLE on ocfs2 - v2 @ 2011-05-19 11:04 ` Christoph Hellwig 0 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2011-05-19 11:04 UTC (permalink / raw) To: Sunil Mushran Cc: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, ocfs2-devel On Wed, May 18, 2011 at 07:44:42PM -0700, Sunil Mushran wrote: > It is improved since the last post. It runs cleanly on zfs, ocfs2 and ext3 > (default behavior). Users testing on zfs will need to flip the values of > SEEK_HOLE/DATA. sounds like we should switch the around, just to cause the least amount of confusion. ^ permalink raw reply [flat|nested] 18+ messages in thread
* PATCH v3: Add support SEEK_DATA/HOLE in ocfs2 @ 2011-05-23 23:55 Sunil Mushran 2011-05-23 23:55 ` [PATCH] ocfs2: Implement llseek() Sunil Mushran 0 siblings, 1 reply; 18+ messages in thread From: Sunil Mushran @ 2011-05-23 23:55 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, o The following patch implements SEEK_DATA/HOLE on ocfs2. The patch has been tested atop Josef's v4 patch series. Logs of successful runs on btrfs, ext3, ext4 and ocfs2 are available here: http://oss.oracle.com/~smushran/seek_data/ Thanks Sunil ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] ocfs2: Implement llseek() 2011-05-23 23:55 PATCH v3: Add support SEEK_DATA/HOLE in ocfs2 Sunil Mushran @ 2011-05-23 23:55 ` Sunil Mushran 0 siblings, 0 replies; 18+ messages in thread From: Sunil Mushran @ 2011-05-23 23:55 UTC (permalink / raw) To: josef, linux-kernel, linux-fsdevel, linux-btrfs, linux-ext4, viro, o ocfs2 implements its own llseek() to provide the SEEK_HOLE/SEEK_DATA functionality. SEEK_HOLE sets the file pointer to the start of either a hole or an unwritten (preallocated) extent, that is greater than or equal to the supplied offset. SEEK_DATA sets the file pointer to the start of an allocated extent (not unwritten) that is greater than or equal to the supplied offset. If the supplied offset is on a desired region, then the file pointer is set to it. Offsets greater than or equal to the file size return -ENXIO. Unwritten (preallocated) extents are considered holes because the file system treats reads to such regions in the same way as it does to holes. Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com> --- fs/ocfs2/extent_map.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++ fs/ocfs2/extent_map.h | 2 + fs/ocfs2/file.c | 53 ++++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 2 deletions(-) diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c index 23457b4..2f5b92e 100644 --- a/fs/ocfs2/extent_map.c +++ b/fs/ocfs2/extent_map.c @@ -832,6 +832,102 @@ out: return ret; } +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret; + unsigned int is_last = 0, is_data = 0; + u16 cs_bits = OCFS2_SB(inode->i_sb)->s_clustersize_bits; + u32 cpos, cend, clen, hole_size; + u64 extoff, extlen; + struct buffer_head *di_bh = NULL; + struct ocfs2_extent_rec rec; + + BUG_ON(origin != SEEK_DATA && origin != SEEK_HOLE); + + ret = ocfs2_inode_lock(inode, &di_bh, 0); + if (ret) { + mlog_errno(ret); + goto out; + } + + down_read(&OCFS2_I(inode)->ip_alloc_sem); + + if (*offset >= inode->i_size) { + ret = -ENXIO; + goto out_unlock; + } + + if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { + if (origin == SEEK_HOLE) + *offset = inode->i_size; + goto out_unlock; + } + + clen = 0; + cpos = *offset >> cs_bits; + cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); + + while (cpos < cend && !is_last) { + ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, + &rec, &is_last); + if (ret) { + mlog_errno(ret); + goto out_unlock; + } + + extoff = cpos; + extoff <<= cs_bits; + + if (rec.e_blkno == 0ULL) { + clen = hole_size; + is_data = 0; + } else { + clen = le16_to_cpu(rec.e_leaf_clusters) - + (cpos - le32_to_cpu(rec.e_cpos)); + is_data = (rec.e_flags & OCFS2_EXT_UNWRITTEN) ? 0 : 1; + } + + if ((!is_data && origin == SEEK_HOLE) || + (is_data && origin == SEEK_DATA)) { + if (extoff > *offset) + *offset = extoff; + goto out_unlock; + } + + if (!is_last) + cpos += clen; + } + + if (origin == SEEK_HOLE) { + extoff = cpos; + extoff <<= cs_bits; + extlen = clen; + extlen <<= cs_bits; + + if ((extoff + extlen) > inode->i_size) + extlen = inode->i_size - extoff; + extoff += extlen; + if (extoff > *offset) + *offset = extoff; + goto out_unlock; + } + + ret = -ENXIO; + +out_unlock: + + brelse(di_bh); + + up_read(&OCFS2_I(inode)->ip_alloc_sem); + + ocfs2_inode_unlock(inode, 0); +out: + if (ret && ret != -ENXIO) + ret = -ENXIO; + return ret; +} + int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr, struct buffer_head *bhs[], int flags, int (*validate)(struct super_block *sb, diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h index e79d41c..67ea57d 100644 --- a/fs/ocfs2/extent_map.h +++ b/fs/ocfs2/extent_map.h @@ -53,6 +53,8 @@ int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 map_start, u64 map_len); +int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int origin); + int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster, u32 *p_cluster, u32 *num_clusters, struct ocfs2_extent_list *el, diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index b1e35a3..13f7d03 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -2584,6 +2584,55 @@ bail: return ret; } +/* Refer generic_file_llseek_unlocked() */ +static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret = 0; + + mutex_lock(&inode->i_mutex); + + switch (origin) { + case SEEK_END: + offset += inode->i_size; + break; + case SEEK_CUR: + if (offset == 0) { + offset = file->f_pos; + goto out; + } + offset += file->f_pos; + break; + case SEEK_DATA: + case SEEK_HOLE: + ret = ocfs2_seek_data_hole_offset(file, &offset, origin); + if (ret) + goto out; + break; + default: + ret = -EINVAL; + goto out; + } + + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) + ret = -EINVAL; + if (!ret && offset > inode->i_sb->s_maxbytes) + ret = -EINVAL; + if (ret) + goto out; + + if (offset != file->f_pos) { + file->f_pos = offset; + file->f_version = 0; + } + +out: + mutex_unlock(&inode->i_mutex); + if (ret) + return ret; + return offset; +} + const struct inode_operations ocfs2_file_iops = { .setattr = ocfs2_setattr, .getattr = ocfs2_getattr, @@ -2606,7 +2655,7 @@ const struct inode_operations ocfs2_special_file_iops = { * ocfs2_fops_no_plocks and ocfs2_dops_no_plocks! */ const struct file_operations ocfs2_fops = { - .llseek = generic_file_llseek, + .llseek = ocfs2_file_llseek, .read = do_sync_read, .write = do_sync_write, .mmap = ocfs2_mmap, @@ -2654,7 +2703,7 @@ const struct file_operations ocfs2_dops = { * the cluster. */ const struct file_operations ocfs2_fops_no_plocks = { - .llseek = generic_file_llseek, + .llseek = ocfs2_file_llseek, .read = do_sync_read, .write = do_sync_write, .mmap = ocfs2_mmap, -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2011-05-23 23:55 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-05-19 2:44 SEEK_DATA/HOLE on ocfs2 - v2 Sunil Mushran 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran 2011-05-19 2:44 ` [PATCH] fs: Fix default SEEK_DATA and SEEK_HOLE implementation Sunil Mushran 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran 2011-05-19 2:44 ` [PATCH] ocfs2: Implement llseek() Sunil Mushran 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran 2011-05-19 9:13 ` Tristan Ye 2011-05-19 9:13 ` [Ocfs2-devel] " Tristan Ye 2011-05-19 9:13 ` Tristan Ye 2011-05-19 17:45 ` Sunil Mushran 2011-05-19 17:45 ` Sunil Mushran 2011-05-19 11:05 ` Christoph Hellwig 2011-05-19 11:05 ` Christoph Hellwig 2011-05-19 17:29 ` Sunil Mushran 2011-05-19 17:29 ` Sunil Mushran 2011-05-19 11:04 ` [Ocfs2-devel] SEEK_DATA/HOLE on ocfs2 - v2 Christoph Hellwig 2011-05-19 11:04 ` Christoph Hellwig 2011-05-23 23:55 PATCH v3: Add support SEEK_DATA/HOLE in ocfs2 Sunil Mushran 2011-05-23 23:55 ` [PATCH] ocfs2: Implement llseek() Sunil Mushran
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.