Skip to contentSkip to conversation

GFP flags and allocation cleanups

PATCH New David Sterba 1 participant 6 messages Jul 14, 2017 Btrfs file system

lore
Share & tools
lore

David Sterba

author maintainer
#0 lore raw

cover GFP flags and allocation cleanups

A few more NOFS -> KERNEL conversions and simplification of memory allocations.

David Sterba (5):
btrfs: use GFP_KERNEL in mount and remount
btrfs: use GFP_KERNEL in btrfs_defrag_file
btrfs: defrag: make readahead state allocation failure non-fatal
btrfs: factor reading progress out of btrfs_dev_replace_status
btrfs: simplify btrfs_dev_replace_kthread

 fs/btrfs/dev-replace.c | 74 ++++++++++++++++++++++++++++----------------------
 fs/btrfs/ioctl.c       | 20 +++++++-------
 fs/btrfs/super.c       | 15 +++++-----
 3 files changed, 59 insertions(+), 50 deletions(-)
Signature · 1 line
2.13.0
author maintainer
#1 lore raw

PATCH 1/5 btrfs: use GFP_KERNEL in mount and remount

We don't need to restrict the allocation flags in btrfs_mount or _remount. No big filesystem locks are held (possibly s_umount but that does no count here).

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/super.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
fs/btrfs/super.c +8 -7
index 74e47794e63f..35aae1c0377b 100644
@@ -425,7 +425,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 	 * strsep changes the string, duplicate it because parse_options
 	 * gets called twice
 	 */
-	options = kstrdup(options, GFP_NOFS);
+	options = kstrdup(options, GFP_KERNEL);
 	if (!options)
 		return -ENOMEM;
 

@@ -949,7 +949,7 @@ static char *get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
 	}
 	path->leave_spinning = 1;
 
-	name = kmalloc(PATH_MAX, GFP_NOFS);
+	name = kmalloc(PATH_MAX, GFP_KERNEL);
 	if (!name) {
 		ret = -ENOMEM;
 		goto err;

@@ -1336,10 +1336,11 @@ static char *setup_root_args(char *args)
 	char *buf, *dst, *sep;
 
 	if (!args)
-		return kstrdup("subvolid=0", GFP_NOFS);
+		return kstrdup("subvolid=0", GFP_KERNEL);
 
 	/* The worst case is that we add ",subvolid=0" to the end. */
-	buf = dst = kmalloc(strlen(args) + strlen(",subvolid=0") + 1, GFP_NOFS);
+	buf = dst = kmalloc(strlen(args) + strlen(",subvolid=0") + 1,
+			GFP_KERNEL);
 	if (!buf)
 		return NULL;
 

@@ -1568,7 +1569,7 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 	 * it for searching for existing supers, so this lets us do that and
 	 * then open_ctree will properly initialize everything later.
 	 */
-	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
+	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_KERNEL);
 	if (!fs_info) {
 		error = -ENOMEM;
 		goto error_sec_opts;

@@ -1576,8 +1577,8 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 
 	fs_info->fs_devices = fs_devices;
 
-	fs_info->super_copy = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_NOFS);
-	fs_info->super_for_commit = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_NOFS);
+	fs_info->super_copy = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
+	fs_info->super_for_commit = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
 	security_init_mnt_opts(&fs_info->security_opts);
 	if (!fs_info->super_copy || !fs_info->super_for_commit) {
 		error = -ENOMEM;
Signature · 1 line
2.13.0
author maintainer
#2 lore raw

PATCH 2/5 btrfs: use GFP_KERNEL in btrfs_defrag_file

In reply to David Sterba

We can safely use GFP_KERNEL, the function is called from two contexts:

- ioctl handler, called directly, no locks taken
- cleaner thread, running all queued defrag work, outside of any locks

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ioctl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
fs/btrfs/ioctl.c +2 -3
index fa1b78cf25f6..d260c4c57338 100644
@@ -1308,7 +1308,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 	 * context
 	 */
 	if (!file) {
-		ra = kzalloc(sizeof(*ra), GFP_NOFS);
+		ra = kzalloc(sizeof(*ra), GFP_KERNEL);
 		if (!ra)
 			return -ENOMEM;
 		file_ra_state_init(ra, inode->i_mapping);

@@ -1316,8 +1316,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 		ra = &file->f_ra;
 	}
 
-	pages = kmalloc_array(max_cluster, sizeof(struct page *),
-			GFP_NOFS);
+	pages = kmalloc_array(max_cluster, sizeof(struct page *), GFP_KERNEL);
 	if (!pages) {
 		ret = -ENOMEM;
 		goto out_ra;
Signature · 1 line
2.13.0
author maintainer
#3 lore raw

PATCH 3/5 btrfs: defrag: make readahead state allocation failure non-fatal

In reply to David Sterba

All sorts of readahead errors are not considered fatal. We can continue defragmentation without it, with some potential slow down, which will last only for the current inode.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ioctl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
fs/btrfs/ioctl.c +8 -7
index d260c4c57338..f418d29b0b92 100644
@@ -1304,14 +1304,14 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 		extent_thresh = SZ_256K;
 
 	/*
-	 * if we were not given a file, allocate a readahead
-	 * context
+	 * If we were not given a file, allocate a readahead context. As
+	 * readahead is just an optimization, defrag will work without it so
+	 * we don't error out.
 	 */
 	if (!file) {
 		ra = kzalloc(sizeof(*ra), GFP_KERNEL);
-		if (!ra)
-			return -ENOMEM;
-		file_ra_state_init(ra, inode->i_mapping);
+		if (ra)
+			file_ra_state_init(ra, inode->i_mapping);
 	} else {
 		ra = &file->f_ra;
 	}

@@ -1394,8 +1394,9 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 
 		if (i + cluster > ra_index) {
 			ra_index = max(i, ra_index);
-			btrfs_force_ra(inode->i_mapping, ra, file, ra_index,
-				       cluster);
+			if (ra)
+				btrfs_force_ra(inode->i_mapping, ra, file,
+						ra_index, cluster);
 			ra_index += cluster;
 		}
Signature · 1 line
2.13.0
author maintainer
#4 lore raw

PATCH 4/5 btrfs: factor reading progress out of btrfs_dev_replace_status

In reply to David Sterba

We'll want to read the percentage value from dev_replace elsewhere, move the logic to a separate helper.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/dev-replace.c | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)
fs/btrfs/dev-replace.c +30 -16
index bee3edeea7a3..31d01eefecd0 100644
@@ -639,11 +639,39 @@ static void btrfs_dev_replace_update_device_in_mapping_tree(
 	write_unlock(&em_tree->lock);
 }
 
+/*
+ * Read progress of device replace status according to the state and last
+ * stored position. The value format is the same as for
+ * btrfs_dev_replace::progress_1000
+ */
+static u64 btrfs_dev_replace_progress(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
+	u64 ret = 0;
+
+	switch (dev_replace->replace_state) {
+	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
+	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
+		ret = 0;
+		break;
+	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
+		ret = 1000;
+		break;
+	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
+	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
+		ret = div64_u64(dev_replace->cursor_left,
+				div_u64(btrfs_device_get_total_bytes(
+						dev_replace->srcdev), 1000));
+		break;
+	}
+
+	return ret;
+}
+
 void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 			      struct btrfs_ioctl_dev_replace_args *args)
 {
 	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
-	struct btrfs_device *srcdev;
 
 	btrfs_dev_replace_lock(dev_replace, 0);
 	/* even if !dev_replace_is_valid, the values are good enough for

@@ -656,21 +684,7 @@ void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
 		atomic64_read(&dev_replace->num_write_errors);
 	args->status.num_uncorrectable_read_errors =
 		atomic64_read(&dev_replace->num_uncorrectable_read_errors);
-	switch (dev_replace->replace_state) {
-	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
-	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		args->status.progress_1000 = 0;
-		break;
-	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
-		args->status.progress_1000 = 1000;
-		break;
-	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
-	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
-		srcdev = dev_replace->srcdev;
-		args->status.progress_1000 = div64_u64(dev_replace->cursor_left,
-			div_u64(btrfs_device_get_total_bytes(srcdev), 1000));
-		break;
-	}
+	args->status.progress_1000 = btrfs_dev_replace_progress(fs_info);
 	btrfs_dev_replace_unlock(dev_replace, 0);
 }
Signature · 1 line
2.13.0
author maintainer
#5 lore raw

PATCH 5/5 btrfs: simplify btrfs_dev_replace_kthread

In reply to David Sterba

This function prints an informative message and then continues dev-replace. The message contains a progress percentage which is read from the status. The status is allocated dynamically, about 2600 bytes, just to read the single value. That's an overkill. We'll use the new helper and drop the allocation.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/dev-replace.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
fs/btrfs/dev-replace.c +11 -17
index 31d01eefecd0..7a93a3e1a847 100644
@@ -809,25 +809,19 @@ static int btrfs_dev_replace_kthread(void *data)
 {
 	struct btrfs_fs_info *fs_info = data;
 	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
-	struct btrfs_ioctl_dev_replace_args *status_args;
 	u64 progress;
 
-	status_args = kzalloc(sizeof(*status_args), GFP_KERNEL);
-	if (status_args) {
-		btrfs_dev_replace_status(fs_info, status_args);
-		progress = status_args->status.progress_1000;
-		kfree(status_args);
-		progress = div_u64(progress, 10);
-		btrfs_info_in_rcu(fs_info,
-			"continuing dev_replace from %s (devid %llu) to %s @%u%%",
-			dev_replace->srcdev->missing ? "<missing disk>" :
-			rcu_str_deref(dev_replace->srcdev->name),
-			dev_replace->srcdev->devid,
-			dev_replace->tgtdev ?
-			rcu_str_deref(dev_replace->tgtdev->name) :
-			"<missing target disk>",
-			(unsigned int)progress);
-	}
+	progress = btrfs_dev_replace_progress(fs_info);
+	progress = div_u64(progress, 10);
+	btrfs_info_in_rcu(fs_info,
+		"continuing dev_replace from %s (devid %llu) to %s @%u%%",
+		dev_replace->srcdev->missing ? "<missing disk>"
+			: rcu_str_deref(dev_replace->srcdev->name),
+		dev_replace->srcdev->devid,
+		dev_replace->tgtdev ? rcu_str_deref(dev_replace->tgtdev->name)
+			: "<missing target disk>",
+		(unsigned int)progress);
+
 	btrfs_dev_replace_continue_on_mount(fs_info);
 	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
Signature · 1 line
2.13.0