nilfs2: add support for FITRIM ioctl

PATCH In review Andreas Rohner 2 participants 17 messages Feb 15, 2014 – Feb 17, 2014 13 messages not in the archive NILFS2 filesystem

lore

Share & tools

lore

Andreas Rohner

author

2014-02-15 13:34 UTC #0 lore raw

cover nilfs2: add support for FITRIM ioctl

Hi,

This patch adds support for the FITRIM ioctl, which allows user space tools like fstrim to issue TRIM/DISCARD requests to the underlying device. It takes a fstrim_range structure as a parameter and for every clean segment in the specified range the function blkdev_issue_discard is called.

Best regards,
Andreas Rohner

---
Andreas Rohner (2):
nilfs2: add nilfs_sufile_trim_fs to trim clean segs
nilfs2: add FITRIM ioctl support for nilfs2

 fs/nilfs2/ioctl.c  |  46 +++++++++++++++++++++
 fs/nilfs2/sufile.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |   1 +
 3 files changed, 161 insertions(+)

Signature · 6 lines

1.8.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andreas Rohner

author

2014-02-15 13:34 UTC #2 lore raw

PATCH 1/2 nilfs2: add nilfs_sufile_trim_fs to trim clean segs

This patch adds the nilfs_sufile_trim_fs function, which takes a fstrim_range structure and calls blkdev_issue_discard for every clean segment in the specified range.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/sufile.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |   1 +
 2 files changed, 115 insertions(+)

fs/nilfs2/sufile.c +114 -0

index 3127e9f..022b17f 100644
@@ -870,6 +870,120 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 }
 
 /**
+ * nilfs_sufile_trim_fs() - trim ioctl handle function
+ * @sufile: inode of segment usage file
+ * @range: fstrim_range structure
+ *
+ * start:	First Byte to trim
+ * len:		number of Bytes to trim from start
+ * minlen:	minimum extent length in Bytes
+ *
+ * Decription: nilfs_sufile_trim_fs goes through all segments containing bytes
+ * from start to start+len. For each clean segment blkdev_issue_discard
+ * function is invoked to trim it.
+ *
+ * Return Value: On success, 0 is returned or negative error code, otherwise.
+ */
+int nilfs_sufile_trim_fs(struct inode *sufile, struct fstrim_range *range)
+{
+	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
+	struct buffer_head *su_bh;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	size_t susz = NILFS_MDT(sufile)->mi_entry_size;
+	ssize_t n;
+	sector_t seg_start, seg_end, start = 0, nblocks = 0;
+	u64 segnum, end, minlen, trimmed = 0;
+	int i, ret = 0;
+	unsigned long segusages_per_block;
+	unsigned int sects_per_block;
+
+	segusages_per_block = nilfs_sufile_segment_usages_per_block(sufile);
+	sects_per_block = (1 << nilfs->ns_blocksize_bits) /
+		bdev_logical_block_size(nilfs->ns_bdev);
+	segnum = nilfs_get_segnum_of_block(nilfs, range->start >>
+			nilfs->ns_blocksize_bits);
+	end = nilfs_get_segnum_of_block(nilfs, (range->start + range->len)
+			>> nilfs->ns_blocksize_bits);
+	minlen = range->minlen >> nilfs->ns_blocksize_bits;
+
+	if (minlen > nilfs->ns_blocks_per_segment ||
+	    segnum >= nilfs->ns_nsegments ||
+	    range->len < nilfs->ns_blocksize)
+		return -EINVAL;
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
+	if (end == segnum)
+		goto out;
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
+		ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0,
+							   &su_bh);
+		if (ret < 0) {
+			if (ret != -ENOENT)
+				goto out_sem;
+			/* hole */
+			segnum += n;
+			continue;
+		}
+
+		kaddr = kmap(su_bh->b_page);
+		su = nilfs_sufile_block_get_segment_usage(
+			sufile, segnum, su_bh, kaddr);
+		for (i = 0; i < n; ++i, ++segnum, su = (void *)su + susz) {
+			if (!nilfs_segment_usage_clean(su))
+				continue;
+
+			nilfs_get_segment_range(nilfs, segnum, &seg_start,
+						&seg_end);
+
+			if (!nblocks) {
+				start = seg_start;
+				nblocks = seg_end - seg_start + 1;
+			} else if (start + nblocks == seg_start) {
+				nblocks += seg_end - seg_start + 1;
+			} else {
+				ret = blkdev_issue_discard(nilfs->ns_bdev,
+						start * sects_per_block,
+						nblocks * sects_per_block,
+						GFP_NOFS, 0);
+				if (ret < 0) {
+					kunmap(kaddr);
+					put_bh(su_bh);
+					goto out_sem;
+				}
+
+				trimmed += nblocks;
+				nblocks = 0;
+			}
+		}
+		kunmap(kaddr);
+		put_bh(su_bh);
+	}
+
+	if (nblocks) {
+		ret = blkdev_issue_discard(nilfs->ns_bdev,
+					   start * sects_per_block,
+					   nblocks * sects_per_block,
+					   GFP_NOFS, 0);
+		if (!ret)
+			trimmed += nblocks;
+	}
+
+out_sem:
+	up_read(&NILFS_MDT(sufile)->mi_sem);
+out:
+	range->len = trimmed << nilfs->ns_blocksize_bits;
+	return ret;
+}
+
+/**
  * nilfs_sufile_read - read or get sufile inode
  * @sb: super block instance
  * @susize: size of a segment usage entry

fs/nilfs2/sufile.h +1 -0

index e84bc5b..2434abd 100644
@@ -65,6 +65,7 @@ void nilfs_sufile_do_set_error(struct inode *, __u64, struct buffer_head *,
 int nilfs_sufile_resize(struct inode *sufile, __u64 newnsegs);
 int nilfs_sufile_read(struct super_block *sb, size_t susize,
 		      struct nilfs_inode *raw_inode, struct inode **inodep);
+int nilfs_sufile_trim_fs(struct inode *sufile, struct fstrim_range *range);
 
 /**
  * nilfs_sufile_scrap - make a segment garbage

Signature · 6 lines

1.8.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 02d463460388a9bdc1413aa4d6fdfa5a402b9452.1392470644.git.andreas.rohner-hi6Y0CQ0nG0@public.gmane.org

Ryusuke Konishi

2014-02-17 03:00 UTC #4 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Hi Andreas,

Quoted 5 lines · Andreas Rohner

This patch adds the nilfs_sufile_trim_fs function, which takes a fstrim_range structure and calls blkdev_issue_discard for every clean segment in the specified range.
Signed-off-by Andreas Rohner <[email protected]>

Thank you for doing this work.
I'll add comments inline below.

Quoted 41 lines

---
 fs/nilfs2/sufile.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |   1 +
 2 files changed, 115 insertions(+)

fs/nilfs2/sufile.c +28 -0

index 3127e9f..022b17f 100644
@@ -870,6 +870,120 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 }
 
 /**
+ * nilfs_sufile_trim_fs() - trim ioctl handle function
+ * @sufile: inode of segment usage file
+ * @range: fstrim_range structure
+ *
+ * start:	First Byte to trim
+ * len:		number of Bytes to trim from start
+ * minlen:	minimum extent length in Bytes
+ *
+ * Decription: nilfs_sufile_trim_fs goes through all segments containing bytes
+ * from start to start+len. For each clean segment blkdev_issue_discard
+ * function is invoked to trim it.
+ *
+ * Return Value: On success, 0 is returned or negative error code, otherwise.
+ */
+int nilfs_sufile_trim_fs(struct inode *sufile, struct fstrim_range *range)
+{
+	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
+	struct buffer_head *su_bh;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	size_t susz = NILFS_MDT(sufile)->mi_entry_size;
+	ssize_t n;
+	sector_t seg_start, seg_end, start = 0, nblocks = 0;
+	u64 segnum, end, minlen, trimmed = 0;
+	int i, ret = 0;
+	unsigned long segusages_per_block;
+	unsigned int sects_per_block;
+

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 910

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+	segusages_per_block = nilfs_sufile_segment_usages_per_block(sufile);
+	sects_per_block = (1 << nilfs->ns_blocksize_bits) /
+		bdev_logical_block_size(nilfs->ns_bdev);
+	segnum = nilfs_get_segnum_of_block(nilfs, range->start >>
+			nilfs->ns_blocksize_bits);
+	end = nilfs_get_segnum_of_block(nilfs, (range->start + range->len)
+			>> nilfs->ns_blocksize_bits);
+	minlen = range->minlen >> nilfs->ns_blocksize_bits;
+
+	if (minlen > nilfs->ns_blocks_per_segment ||

This check looks inappropriate. If we set a lower limit for range->minlin, I think it should be the file system size (block device size). For your information, the meaning of minlen is described as follows in the man page of fstrim command:

Minimum contiguous free range to discard, in bytes. (This value is internally rounded up to a multiple of the filesystem block size). Free ranges smaller than this will be ignored. By increasing this value, the fstrim operation will complete more quickly for filesystems with badly fragmented freespace, although not all blocks will be discarded. Default value is zero, discard every free block.

So, too small lower limit interferes purpose of the minlen argument.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 911

Quoted 1 line

fs/nilfs2/sufile.c +1 -0
+	    segnum >= nilfs->ns_nsegments ||

This is bad too, because userland programs usually don't know the segment structure of NILFS. When we specify the partition size to range->len, FITRIM can fail due to this check.

The upper limit of (range->start + range->len) should be the block device size.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 912

Quoted 1 line

fs/nilfs2/sufile.c +1 -0
+	    range->len < nilfs->ns_blocksize)

This looks OK.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 913

Quoted 1 line

fs/nilfs2/sufile.c +1 -0
+		return -EINVAL;

I think these EINVAL checks should be move to ioctl interface side because they should be performed in a perspective of general file system and shouldn't depend on the internal structure of NILFS so much. If we need to adjust the range in this function, we should do it silently.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 915

Quoted 2 lines

fs/nilfs2/sufile.c +2 -0

+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;

Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:

if (end >= nilfs->ns_nsegments)

	end = nilfs->ns_nsegments - 1;

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 917

Quoted 2 lines

fs/nilfs2/sufile.c +2 -0
+	if (end == segnum)
+		goto out;

and

if (end < segnum)

	goto out;

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 921

Quoted 4 lines

fs/nilfs2/sufile.c +4 -0

+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {

and

  while (segnum <= end) {

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 925

Quoted 4 lines

fs/nilfs2/sufile.c +4 -0

+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);

Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 933

Quoted 8 lines

fs/nilfs2/sufile.c +8 -0

+		ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0,
+							   &su_bh);
+		if (ret < 0) {
+			if (ret != -ENOENT)
+				goto out_sem;
+			/* hole */
+			segnum += n;
+			continue;

Skipping segments within segment usage blocks which are hole, are OK since they are never written after the previous mkfs and mkfs.nilfs2 discards segments.

However, note that this also separates the extent defined with (start, nblocks). So, (start, nblocks) should be updated and blkdev_issue_discard() should be called as needed.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 936

Quoted 3 lines

fs/nilfs2/sufile.c +3 -0
+		}
+
+		kaddr = kmap(su_bh->b_page);

Avoid using kmap()/kunmap() (if possible), these are expensive for architectures nowadays which shares virtual address space among processors. I think we can replace them with kmap_atomic() and kunmap_atomic() for this function by carefully releasing the highmem before calling blocking operations such as blkdev_issue_discard() and nilfs_sufile_get_segment_usage_block().

Quoted 5 lines

fs/nilfs2/sufile.c +5 -0

+		su = nilfs_sufile_block_get_segment_usage(
+			sufile, segnum, su_bh, kaddr);
+		for (i = 0; i < n; ++i, ++segnum, su = (void *)su + susz) {
+			if (!nilfs_segment_usage_clean(su))
+				continue;

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 951

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+
+			nilfs_get_segment_range(nilfs, segnum, &seg_start,
+						&seg_end);
+
+			if (!nblocks) {
+				start = seg_start;
+				nblocks = seg_end - seg_start + 1;
+			} else if (start + nblocks == seg_start) {
+				nblocks += seg_end - seg_start + 1;
+			} else {

We should ignore the extent if the size is smaller than range->minlen.

Quoted 53 lines

fs/nilfs2/sufile.c +35 -0

+				ret = blkdev_issue_discard(nilfs->ns_bdev,
+						start * sects_per_block,
+						nblocks * sects_per_block,
+						GFP_NOFS, 0);
+				if (ret < 0) {
+					kunmap(kaddr);
+					put_bh(su_bh);
+					goto out_sem;
+				}
+
+				trimmed += nblocks;
+				nblocks = 0;
+			}
+		}
+		kunmap(kaddr);
+		put_bh(su_bh);
+	}
+
+	if (nblocks) {
+		ret = blkdev_issue_discard(nilfs->ns_bdev,
+					   start * sects_per_block,
+					   nblocks * sects_per_block,
+					   GFP_NOFS, 0);
+		if (!ret)
+			trimmed += nblocks;
+	}
+
+out_sem:
+	up_read(&NILFS_MDT(sufile)->mi_sem);
+out:
+	range->len = trimmed << nilfs->ns_blocksize_bits;
+	return ret;
+}
+
+/**
  * nilfs_sufile_read - read or get sufile inode
  * @sb: super block instance
  * @susize: size of a segment usage entry

fs/nilfs2/sufile.h +1 -0

index e84bc5b..2434abd 100644
@@ -65,6 +65,7 @@ void nilfs_sufile_do_set_error(struct inode *, __u64, struct buffer_head *,
 int nilfs_sufile_resize(struct inode *sufile, __u64 newnsegs);
 int nilfs_sufile_read(struct super_block *sb, size_t susize,
 		      struct nilfs_inode *raw_inode, struct inode **inodep);
+int nilfs_sufile_trim_fs(struct inode *sufile, struct fstrim_range *range);
 
 /**
  * nilfs_sufile_scrap - make a segment garbage

Signature · 1 line

1.8.5.4

Regards,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20140217.120000.397306175.konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

Ryusuke Konishi

2014-02-17 04:27 UTC #6 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 10 lines · Ryusuke Konishi

Hi Andreas,
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 911
Quoted 1 line · Andreas Rohner
fs/nilfs2/sufile.c +1 -0
+	    segnum >= nilfs->ns_nsegments ||
This is bad too, because userland programs usually don't know the segment structure of NILFS. When we specify the partition size to range->len, FITRIM can fail due to this check.
The upper limit of (range->start + range->len) should be the block device size.

I mistook for this. segnum was the start segment number, so what we should do was:

- Return -EINVAL in ioctl side if range->start is larger than the block device size.
- Escape this function without an error setting range->len to zero if segnum >= nilfs->ns_nsegments.

And, range->len should not be limited by the device size because it is set to ULLONG_MAX by default. We have to accept larger range->len values.

Andreas Rohner

author

2014-02-17 09:06 UTC #7 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

In reply to a message not in the archive

Hi Ryusuke,

Quoted 38 lines · Ryusuke Konishi

Quoted 14 lines
fs/nilfs2/sufile.c +14 -0
+int nilfs_sufile_trim_fs(struct inode *sufile, struct fstrim_range *range)
+{
+	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
+	struct buffer_head *su_bh;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	size_t susz = NILFS_MDT(sufile)->mi_entry_size;
+	ssize_t n;
+	sector_t seg_start, seg_end, start = 0, nblocks = 0;
+	u64 segnum, end, minlen, trimmed = 0;
+	int i, ret = 0;
+	unsigned long segusages_per_block;
+	unsigned int sects_per_block;
+
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 910
Quoted 10 lines
fs/nilfs2/sufile.c +10 -0
+	segusages_per_block = nilfs_sufile_segment_usages_per_block(sufile);
+	sects_per_block = (1 << nilfs->ns_blocksize_bits) /
+		bdev_logical_block_size(nilfs->ns_bdev);
+	segnum = nilfs_get_segnum_of_block(nilfs, range->start >>
+			nilfs->ns_blocksize_bits);
+	end = nilfs_get_segnum_of_block(nilfs, (range->start + range->len)
+			>> nilfs->ns_blocksize_bits);
+	minlen = range->minlen >> nilfs->ns_blocksize_bits;
+
+	if (minlen > nilfs->ns_blocks_per_segment ||
This check looks inappropriate. If we set a lower limit for range->minlin, I think it should be the file system size (block device size). For your information, the meaning of minlen is described as follows in the man page of fstrim command:
Minimum contiguous free range to discard, in bytes. (This value is internally rounded up to a multiple of the filesystem block size). Free ranges smaller than this will be ignored. By increasing this value, the fstrim operation will complete more quickly for filesystems with badly fragmented freespace, although not all blocks will be discarded. Default value is zero, discard every free block.

Quoted 1 line

So, too small lower limit interferes purpose of the minlen argument.

Agreed.

Quoted 8 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 911
Quoted 1 line
fs/nilfs2/sufile.c +1 -0
+	    segnum >= nilfs->ns_nsegments ||
This is bad too, because userland programs usually don't know the segment structure of NILFS. When we specify the partition size to range->len, FITRIM can fail due to this check.
The upper limit of (range->start + range->len) should be the block device size.

ext4 also checks the range structure like that. Besides couldn't it be possible, that the block device is bigger than the file system?

Quoted 11 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 912
Quoted 1 line
fs/nilfs2/sufile.c +1 -0
+	    range->len < nilfs->ns_blocksize)
This looks OK.
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 913
Quoted 1 line
fs/nilfs2/sufile.c +1 -0
+		return -EINVAL;
I think these EINVAL checks should be move to ioctl interface side because they should be performed in a perspective of general file system and shouldn't depend on the internal structure of NILFS so much. If we need to adjust the range in this function, we should do it silently.

I just did it the same way as it is done in ext4.

Quoted 26 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 915
Quoted 2 lines
fs/nilfs2/sufile.c +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 917
Quoted 2 lines
fs/nilfs2/sufile.c +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 921
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {

Quoted 7 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 925
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.

Yes we can do this, if you want to use nilfs_sufile_segment_usages_in_block(). I would just like to state, that my code is also correct here.

Quoted 14 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 933
Quoted 9 lines
fs/nilfs2/sufile.c +8 -0
+		ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0,
+							   &su_bh);
+		if (ret < 0) {
+			if (ret != -ENOENT)
+				goto out_sem;
+			/* hole */
+			segnum += n;
+			continue;
Skipping segments within segment usage blocks which are hole, are OK since they are never written after the previous mkfs and mkfs.nilfs2 discards segments.

Quoted 3 lines

However, note that this also separates the extent defined with (start, nblocks). So, (start, nblocks) should be updated and blkdev_issue_discard() should be called as needed.

As far as I can tell this is not necessary here. But I checked the function again and I found a small bug further below.

Quoted 10 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 936
Quoted 3 lines
fs/nilfs2/sufile.c +3 -0
+		}
+
+		kaddr = kmap(su_bh->b_page);
Avoid using kmap()/kunmap() (if possible), these are expensive for architectures nowadays which shares virtual address space among processors. I think we can replace them with kmap_atomic() and kunmap_atomic() for this function by carefully releasing the highmem before calling blocking operations such as blkdev_issue_discard() and nilfs_sufile_get_segment_usage_block().

Ok. Yes that should be possible.

Quoted 9 lines

fs/nilfs2/sufile.c +9 -0

+		su = nilfs_sufile_block_get_segment_usage(
+			sufile, segnum, su_bh, kaddr);
+		for (i = 0; i < n; ++i, ++segnum, su = (void *)su + susz) {
+			if (!nilfs_segment_usage_clean(su))
+				continue;
+
+			nilfs_get_segment_range(nilfs, segnum, &seg_start,
+						&seg_end);
+

Quoted 8 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 951
Quoted 6 lines
fs/nilfs2/sufile.c +6 -0
+			if (!nblocks) {
+				start = seg_start;
+				nblocks = seg_end - seg_start + 1;
+			} else if (start + nblocks == seg_start) {
+				nblocks += seg_end - seg_start + 1;
+			} else {
We should ignore the extent if the size is smaller than range->minlen.

Agreed.

Quoted 4 lines

fs/nilfs2/sufile.c +4 -0

+				ret = blkdev_issue_discard(nilfs->ns_bdev,
+						start * sects_per_block,
+						nblocks * sects_per_block,
+						GFP_NOFS, 0);

Quoted 8 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 963
Quoted 8 lines
fs/nilfs2/sufile.c +8 -0
+				if (ret < 0) {
+					kunmap(kaddr);
+					put_bh(su_bh);
+					goto out_sem;
+				}
+
+				trimmed += nblocks;
+				nblocks = 0;

This should be:

+ start = seg_start;
+ nblocks = seg_end - seg_start + 1;

Regards,
Andreas Rohner

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ryusuke Konishi

2014-02-17 10:42 UTC #9 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 9 lines · Andreas Rohner

Hi Ryusuke,
Quoted 6 lines · Ryusuke Konishi
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 911
Quoted 1 line
fs/nilfs2/sufile.c +1 -0
+	    segnum >= nilfs->ns_nsegments ||
This is bad too, because userland programs usually don't know the segment structure of NILFS. When we specify the partition size to range->len, FITRIM can fail due to this check.

Quoted 5 lines

Quoted 2 lines
The upper limit of (range->start + range->len) should be the block device size.
ext4 also checks the range structure like that. Besides couldn't it be possible, that the block device is bigger than the file system?

As I mentioned in my second mail, I misunderstood the meaning of this check at first.

As you pointed out, the block device size may be bigger than the file system size after shrinking the file system. So, sbp->s_dev_size should be used for for this check. But it is a bit complicated since the current nilfs object doesn't have on-memory copy of it.

It is OK to use nilfs->ns_nsegments as its alternative.

Quoted 15 lines

Quoted 4 lines
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+				ret = blkdev_issue_discard(nilfs->ns_bdev,
+						start * sects_per_block,
+						nblocks * sects_per_block,
+						GFP_NOFS, 0);
Quoted 8 lines
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 963
Quoted 8 lines
fs/nilfs2/sufile.c +8 -0
+				if (ret < 0) {
+					kunmap(kaddr);
+					put_bh(su_bh);
+					goto out_sem;
+				}
+
+				trimmed += nblocks;
+				nblocks = 0;
This should be:

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 948

Quoted 2 lines

fs/nilfs2/sufile.c +2 -0

+				start = seg_start;
+				nblocks = seg_end - seg_start + 1;

Yes, that's right.

A message that is not in the archive (sent off-list or not yet received): 20140217.194200.431478798.konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

Andreas Rohner

author

2014-02-17 11:04 UTC #11 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 19 lines · Ryusuke Konishi

Quoted 9 lines · Andreas Rohner
Hi Ryusuke,
Quoted 6 lines · Ryusuke Konishi
Review
Quoted 1 line
(quoted diff) +1 -0
+	    segnum >= nilfs->ns_nsegments ||
This is bad too, because userland programs usually don't know the segment structure of NILFS. When we specify the partition size to range->len, FITRIM can fail due to this check.
Quoted 5 lines
Quoted 2 lines
The upper limit of (range->start + range->len) should be the block device size.
ext4 also checks the range structure like that. Besides couldn't it be possible, that the block device is bigger than the file system?
As I mentioned in my second mail, I misunderstood the meaning of this check at first.

Quoted 6 lines

As you pointed out, the block device size may be bigger than the file system size after shrinking the file system. So, sbp->s_dev_size should be used for for this check. But it is a bit complicated since the current nilfs object doesn't have on-memory copy of it.
It is OK to use nilfs->ns_nsegments as its alternative.

Ok, just to be clear. According to your second mail it should look something like this in nilfs_sufile_trim_fs():

	if (segnum >= nilfs->ns_nsegments)
		goto out;

And in nilfs_ioctl_trim_fs():

	if (range.len < nilfs->ns_blocksize ||
		range.start >= (nilfs->ns_nsegments *
			nilfs->ns_blocks_per_segment) <<
			nilfs->ns_blocksize_bits)
		return -EINVAL;

Is that correct?

Regards,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ryusuke Konishi

2014-02-17 11:39 UTC #13 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 35 lines · Andreas Rohner

Quoted 19 lines · Ryusuke Konishi
Quoted 9 lines · Andreas Rohner
Hi Ryusuke,
Quoted 6 lines · Ryusuke Konishi
Review
Quoted 1 line
(quoted diff) +1 -0
+	    segnum >= nilfs->ns_nsegments ||
This is bad too, because userland programs usually don't know the segment structure of NILFS. When we specify the partition size to range->len, FITRIM can fail due to this check.
Quoted 5 lines
Quoted 2 lines
The upper limit of (range->start + range->len) should be the block device size.
ext4 also checks the range structure like that. Besides couldn't it be possible, that the block device is bigger than the file system?
As I mentioned in my second mail, I misunderstood the meaning of this check at first.
Quoted 6 lines
As you pointed out, the block device size may be bigger than the file system size after shrinking the file system. So, sbp->s_dev_size should be used for for this check. But it is a bit complicated since the current nilfs object doesn't have on-memory copy of it.
It is OK to use nilfs->ns_nsegments as its alternative.
Ok, just to be clear. According to your second mail it should look something like this in nilfs_sufile_trim_fs():
	if (segnum >= nilfs->ns_nsegments)
		goto out;
And in nilfs_ioctl_trim_fs():

Quoted 8 lines

	if (range.len < nilfs->ns_blocksize ||
		range.start >= (nilfs->ns_nsegments *
			nilfs->ns_blocks_per_segment) <<
			nilfs->ns_blocksize_bits)
		return -EINVAL;

Is that correct?

Yes, exactly.

Andreas Rohner

author

2014-02-17 09:17 UTC #14 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

In reply to a message not in the archive

Quoted 26 lines · Ryusuke Konishi

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 915
Quoted 2 lines
fs/nilfs2/sufile.c +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 917
Quoted 2 lines
fs/nilfs2/sufile.c +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 921
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {

Quoted 7 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 925
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.

Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.

br,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ryusuke Konishi

2014-02-17 10:06 UTC #16 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 35 lines · Andreas Rohner

Quoted 26 lines · Ryusuke Konishi
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 915
Quoted 2 lines
fs/nilfs2/sufile.c +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 917
Quoted 2 lines
fs/nilfs2/sufile.c +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 921
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {
Quoted 7 lines
fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 925
Quoted 4 lines
fs/nilfs2/sufile.c +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.

Quoted 4 lines

Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.

The problem seems that 'end' is not calculated properly. I think it should be

end = nilfs_get_segnum_of_block(

		nilfs,
		(range->start + range->len + <segment-size-in-bytes> - 1)
		  >> nilfs->ns_blocksize_bits) - 1;

or can be simplified to

end = nilfs_get_segnum_of_block(

		nilfs,
		(range->start + range->len - 1) >> nilfs->ns_blocksize_bits);

if range->len > 0 is guaranteed.

The calculation of segnum extends the range to be discarded since it is rounded down to segment alignment, but that of the current implementation for 'end' truncates the range. Both should be extended.

A message that is not in the archive (sent off-list or not yet received): 20140217.190610.135800072.konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

Andreas Rohner

author

2014-02-17 10:34 UTC #18 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 55 lines · Ryusuke Konishi

Quoted 35 lines · Andreas Rohner
Quoted 26 lines · Ryusuke Konishi
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
Review
Quoted 4 lines
(quoted diff) +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.
Quoted 4 lines
Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.
The problem seems that 'end' is not calculated properly. I think it should be
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len + <segment-size-in-bytes> - 1)
		  >> nilfs->ns_blocksize_bits) - 1;
or can be simplified to
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len - 1) >> nilfs->ns_blocksize_bits);

Quoted 7 lines

if range->len > 0 is guaranteed.
The calculation of segnum extends the range to be discarded since it is rounded down to segment alignment, but that of the current implementation for 'end' truncates the range. Both should be extended.

Then shouldn't both be truncated? The user expects that less bytes than range->len are discarded but not more. Maybe I am wrong and it is defined somewhere...

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ryusuke Konishi

2014-02-17 11:21 UTC #20 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 59 lines · Andreas Rohner

Quoted 58 lines · Ryusuke Konishi
Quoted 35 lines · Andreas Rohner
Quoted 26 lines · Ryusuke Konishi
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
Review
Quoted 4 lines
(quoted diff) +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.
Quoted 4 lines
Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.
The problem seems that 'end' is not calculated properly. I think it should be
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len + <segment-size-in-bytes> - 1)
		  >> nilfs->ns_blocksize_bits) - 1;
or can be simplified to
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len - 1) >> nilfs->ns_blocksize_bits);
if range->len > 0 is guaranteed.

Quoted 8 lines

Quoted 4 lines
The calculation of segnum extends the range to be discarded since it is rounded down to segment alignment, but that of the current implementation for 'end' truncates the range. Both should be extended.
Then shouldn't both be truncated? The user expects that less bytes than range->len are discarded but not more. Maybe I am wrong and it is defined somewhere...

Uum, xfs looks to be truncating both. This seems to be authentic when we think the original meaning of the fitrim range.

I first thought truncating the range can generate gaps between consecutive calls of FITRIM (if we use FITRIM like that). But now I'm tempted to take the xfs approach. Let's take this semantics.

So, we now have to round up range->start to calculate (start) segnum.

A message that is not in the archive (sent off-list or not yet received): 20140217.202143.05344630.konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

Andreas Rohner

author

2014-02-17 11:53 UTC #22 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 69 lines · Ryusuke Konishi

Quoted 59 lines · Andreas Rohner
Quoted 58 lines · Ryusuke Konishi
Quoted 35 lines · Andreas Rohner
Quoted 26 lines · Ryusuke Konishi
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
Review
Quoted 4 lines
(quoted diff) +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.
Quoted 4 lines
Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.
The problem seems that 'end' is not calculated properly. I think it should be
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len + <segment-size-in-bytes> - 1)
		  >> nilfs->ns_blocksize_bits) - 1;
or can be simplified to
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len - 1) >> nilfs->ns_blocksize_bits);
if range->len > 0 is guaranteed.
Quoted 8 lines
Quoted 4 lines
The calculation of segnum extends the range to be discarded since it is rounded down to segment alignment, but that of the current implementation for 'end' truncates the range. Both should be extended.
Then shouldn't both be truncated? The user expects that less bytes than range->len are discarded but not more. Maybe I am wrong and it is defined somewhere...

Quoted 5 lines

Uum, xfs looks to be truncating both. This seems to be authentic when we think the original meaning of the fitrim range.
I first thought truncating the range can generate gaps between consecutive calls of FITRIM (if we use FITRIM like that).

Hmm good point I didn't think of that. Then by extending both sides the overlapping segments would get discarded twice with consecutive calls, which shouldn't be a problem.

Quoted 4 lines

But now I'm
tempted to take the xfs approach. Let's take this semantics.
So, we now have to round up range->start to calculate (start) segnum.

Ok. I think in the end it doesn't really matter, because most users will use the default values:

range->start = 0
range->minlen = 0
range->len = ULLONG_MAX

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ryusuke Konishi

2014-02-17 13:28 UTC #24 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 83 lines · Andreas Rohner

Quoted 69 lines · Ryusuke Konishi
Quoted 59 lines · Andreas Rohner
Quoted 58 lines · Ryusuke Konishi
Quoted 35 lines · Andreas Rohner
Quoted 26 lines · Ryusuke Konishi
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
Review
Quoted 4 lines
(quoted diff) +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.
Quoted 4 lines
Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.
The problem seems that 'end' is not calculated properly. I think it should be
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len + <segment-size-in-bytes> - 1)
		  >> nilfs->ns_blocksize_bits) - 1;
or can be simplified to
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len - 1) >> nilfs->ns_blocksize_bits);
if range->len > 0 is guaranteed.
Quoted 8 lines
Quoted 4 lines
The calculation of segnum extends the range to be discarded since it is rounded down to segment alignment, but that of the current implementation for 'end' truncates the range. Both should be extended.
Then shouldn't both be truncated? The user expects that less bytes than range->len are discarded but not more. Maybe I am wrong and it is defined somewhere...
Quoted 5 lines
Uum, xfs looks to be truncating both. This seems to be authentic when we think the original meaning of the fitrim range.
I first thought truncating the range can generate gaps between consecutive calls of FITRIM (if we use FITRIM like that).
Hmm good point I didn't think of that. Then by extending both sides the overlapping segments would get discarded twice with consecutive calls, which shouldn't be a problem.
Quoted 3 lines
But now I'm
tempted to take the xfs approach. Let's take this semantics.

Quoted 8 lines

Quoted 1 line
So, we now have to round up range->start to calculate (start) segnum.
Ok. I think in the end it doesn't really matter, because most users will use the default values:
range->start = 0
range->minlen = 0
range->len = ULLONG_MAX

Or, we have the third option to avoid this issue. It's simply truncating both to sector boundary. In this case, the gap problem will not arise since userland programs usually separate the range considering sector boundary or file system block size boundary. I think this is more preferable than truncating it to segment size.

How do you think of this ?

A message that is not in the archive (sent off-list or not yet received): 20140217.222811.27809410.konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

Andreas Rohner

author

2014-02-17 13:51 UTC #26 lore raw

Re: [PATCH 1/2] nilfs2: add nilfs_sufile_trim_fs to trim clean segs

Quoted 93 lines · Ryusuke Konishi

Quoted 83 lines · Andreas Rohner
Quoted 69 lines · Ryusuke Konishi
Quoted 59 lines · Andreas Rohner
Quoted 58 lines · Ryusuke Konishi
Quoted 35 lines · Andreas Rohner
Quoted 26 lines · Ryusuke Konishi
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end > nilfs->ns_nsegments)
+		end = nilfs->ns_nsegments;
Yes, this adjustment is what we should do here, but 'end' segnum was rounded down to segment alighment before. So, it should be:
if (end >= nilfs->ns_nsegments)
	end = nilfs->ns_nsegments - 1;
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (end == segnum)
+		goto out;
and
if (end < segnum)
	goto out;
Review
Quoted 4 lines
(quoted diff) +4 -0
+
+	down_read(&NILFS_MDT(sufile)->mi_sem);
+
+	while (segnum < end) {
and
  while (segnum <= end) {
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		n = min_t(unsigned long,
+			  segusages_per_block -
+				  nilfs_sufile_get_offset(sufile, segnum),
+			  end - segnum);
Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate 'n'.
Quoted 4 lines
Actually I don't think that is correct. What if range->start = 0 and range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard segment 0 and segment 1, whereas my version would discard only segment 0, which seems to be more reasonable.
The problem seems that 'end' is not calculated properly. I think it should be
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len + <segment-size-in-bytes> - 1)
		  >> nilfs->ns_blocksize_bits) - 1;
or can be simplified to
end = nilfs_get_segnum_of_block(
		nilfs,
		(range->start + range->len - 1) >> nilfs->ns_blocksize_bits);
if range->len > 0 is guaranteed.
Quoted 8 lines
Quoted 4 lines
The calculation of segnum extends the range to be discarded since it is rounded down to segment alignment, but that of the current implementation for 'end' truncates the range. Both should be extended.
Then shouldn't both be truncated? The user expects that less bytes than range->len are discarded but not more. Maybe I am wrong and it is defined somewhere...
Quoted 5 lines
Uum, xfs looks to be truncating both. This seems to be authentic when we think the original meaning of the fitrim range.
I first thought truncating the range can generate gaps between consecutive calls of FITRIM (if we use FITRIM like that).
Hmm good point I didn't think of that. Then by extending both sides the overlapping segments would get discarded twice with consecutive calls, which shouldn't be a problem.
Quoted 3 lines
But now I'm
tempted to take the xfs approach. Let's take this semantics.
Quoted 8 lines
Quoted 1 line
So, we now have to round up range->start to calculate (start) segnum.
Ok. I think in the end it doesn't really matter, because most users will use the default values:
range->start = 0
range->minlen = 0
range->len = ULLONG_MAX

Quoted 7 lines

Or, we have the third option to avoid this issue. It's simply truncating both to sector boundary. In this case, the gap problem will not arise since userland programs usually separate the range considering sector boundary or file system block size boundary. I think this is more preferable than truncating it to segment size.
How do you think of this ?

Yes I think that is a good idea. It shouldn't be hard to implement either. I will try it and then send you a new version of the patch.

Regards,
Andreas Rohner

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Rohner

author

2014-02-15 13:34 UTC #27 lore raw

PATCH 2/2 nilfs2: add FITRIM ioctl support for nilfs2

In reply to a message not in the archive

This patch adds support for the FITRIM ioctl, which enables user space tools to issue TRIM/DISCARD requests to the underlying device. Every clean segment within the specified range will be discarded.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/ioctl.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

fs/nilfs2/ioctl.c +46 -0

index 2b34021..cb1bea4 100644
@@ -1072,6 +1072,49 @@ out:
 }
 
 /**
+ * nilfs_ioctl_trim_fs() - trim ioctl handle function
+ * @inode: inode object
+ * @argp: pointer on argument from userspace
+ *
+ * Decription: nilfs_ioctl_trim_fs is the FITRIM ioctl handle function. It
+ * checks the arguments from userspace and calls nilfs_sufile_trim_fs, which
+ * performs the actual trim operation.
+ *
+ * Return Value: On success, 0 is returned or negative error code, otherwise.
+ */
+static int nilfs_ioctl_trim_fs(struct inode *inode, void __user *argp)
+{
+	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
+	struct request_queue *q = bdev_get_queue(nilfs->ns_bdev);
+	struct fstrim_range range;
+	int ret = 0;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (!blk_queue_discard(q))
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&range, argp, sizeof(range)))
+		return -EFAULT;
+
+	range.minlen = max_t(unsigned int, (unsigned int)range.minlen,
+			q->limits.discard_granularity);
+
+	down_read(&nilfs->ns_segctor_sem);
+	ret = nilfs_sufile_trim_fs(nilfs->ns_sufile, &range);
+	up_read(&nilfs->ns_segctor_sem);
+
+	if (ret < 0)
+		return ret;
+
+	if (copy_to_user(argp, &range, sizeof(range)))
+		return -EFAULT;
+
+	return 0;
+}
+
+/**
  * nilfs_ioctl_set_alloc_range - limit range of segments to be allocated
  * @inode: inode object
  * @argp: pointer on argument from userspace

@@ -1205,6 +1248,8 @@ long nilfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return nilfs_ioctl_resize(inode, filp, argp);
 	case NILFS_IOCTL_SET_ALLOC_RANGE:
 		return nilfs_ioctl_set_alloc_range(inode, argp);
+	case FITRIM:
+		return nilfs_ioctl_trim_fs(inode, argp);
 	default:
 		return -ENOTTY;
 	}

@@ -1235,6 +1280,7 @@ long nilfs_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case NILFS_IOCTL_SYNC:
 	case NILFS_IOCTL_RESIZE:
 	case NILFS_IOCTL_SET_ALLOC_RANGE:
+	case FITRIM:
 		break;
 	default:
 		return -ENOIOCTLCMD;

Signature · 6 lines

1.8.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): c1b713f94e9ebdb093c539a4c79a0426c45639a9.1392470644.git.andreas.rohner-hi6Y0CQ0nG0@public.gmane.org

Ryusuke Konishi

2014-02-17 03:01 UTC #29 lore raw

Re: [PATCH 2/2] nilfs2: add FITRIM ioctl support for nilfs2

Quoted 23 lines · Andreas Rohner

This patch adds support for the FITRIM ioctl, which enables user space tools to issue TRIM/DISCARD requests to the underlying device. Every clean segment within the specified range will be discarded.
Signed-off-by Andreas Rohner <[email protected]>
---
 fs/nilfs2/ioctl.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
fs/nilfs2/ioctl.c +6 -0
index 2b34021..cb1bea4 100644
@@ -1072,6 +1072,49 @@ out:
 }
 
 /**
+ * nilfs_ioctl_trim_fs() - trim ioctl handle function
+ * @inode: inode object
+ * @argp: pointer on argument from userspace
+ *
+ * Decription: nilfs_ioctl_trim_fs is the FITRIM ioctl handle function. It
+ * checks the arguments from userspace and calls nilfs_sufile_trim_fs, which

fs/nilfs2/ioctl.c out: · line 1090

Quoted 10 lines

fs/nilfs2/ioctl.c +10 -0

+ * performs the actual trim operation.
+ *
+ * Return Value: On success, 0 is returned or negative error code, otherwise.
+ */
+static int nilfs_ioctl_trim_fs(struct inode *inode, void __user *argp)
+{
+	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
+	struct request_queue *q = bdev_get_queue(nilfs->ns_bdev);
+	struct fstrim_range range;
+	int ret = 0;

This initialization is unnecessary.

Other looks OK to me.

Regards,
Ryusuke Konishi

Quoted 54 lines

fs/nilfs2/ioctl.c +30 -0

+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (!blk_queue_discard(q))
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&range, argp, sizeof(range)))
+		return -EFAULT;
+
+	range.minlen = max_t(unsigned int, (unsigned int)range.minlen,
+			q->limits.discard_granularity);
+
+	down_read(&nilfs->ns_segctor_sem);
+	ret = nilfs_sufile_trim_fs(nilfs->ns_sufile, &range);
+	up_read(&nilfs->ns_segctor_sem);
+
+	if (ret < 0)
+		return ret;
+
+	if (copy_to_user(argp, &range, sizeof(range)))
+		return -EFAULT;
+
+	return 0;
+}
+
+/**
  * nilfs_ioctl_set_alloc_range - limit range of segments to be allocated
  * @inode: inode object
  * @argp: pointer on argument from userspace

@@ -1205,6 +1248,8 @@ long nilfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return nilfs_ioctl_resize(inode, filp, argp);
 	case NILFS_IOCTL_SET_ALLOC_RANGE:
 		return nilfs_ioctl_set_alloc_range(inode, argp);
+	case FITRIM:
+		return nilfs_ioctl_trim_fs(inode, argp);
 	default:
 		return -ENOTTY;
 	}

@@ -1235,6 +1280,7 @@ long nilfs_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case NILFS_IOCTL_SYNC:
 	case NILFS_IOCTL_RESIZE:
 	case NILFS_IOCTL_SET_ALLOC_RANGE:
+	case FITRIM:
 		break;
 	default:
 		return -ENOIOCTLCMD;

Signature · 6 lines

1.8.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html