Skip to contentSkip to conversation

f2fs: avoid migratepage for atomic written page

PATCH In review Jaegeuk Kim 2 participants 4 messages Jul 3, 2017 – Jul 6, 2017

Subsystems (2)
VFS F2FS file system
lore
Share & tools
lore

Jaegeuk Kim

author maintainer
#0 lore raw

PATCH 1/1 f2fs: avoid migratepage for atomic written page

In order to avoid lock contention for atomic written pages, we'd better give EAGAIN in f2fs_migrate_page. We expect it will be released soon as transaction commits.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)
fs/f2fs/data.c +10 -25
index d58b81213a86..1458e3a6d630 100644
@@ -2197,41 +2197,26 @@ static sector_t f2fs_bmap(struct address_space *mapping, sector_t block)
 int f2fs_migrate_page(struct address_space *mapping,
 		struct page *newpage, struct page *page, enum migrate_mode mode)
 {
-	int rc, extra_count;
-	struct f2fs_inode_info *fi = F2FS_I(mapping->host);
-	bool atomic_written = IS_ATOMIC_WRITTEN_PAGE(page);
+	int rc;
 
-	BUG_ON(PageWriteback(page));
-
-	/* migrating an atomic written page is safe with the inmem_lock hold */
-	if (atomic_written && !mutex_trylock(&fi->inmem_lock))
+	/*
+	 * We'd better return EAGAIN for atomic pages, which will be committed
+	 * sooner or later. Don't botter transactions with inmem_lock.
+	 */
+	if (IS_ATOMIC_WRITTEN_PAGE(page))
 		return -EAGAIN;
 
+	BUG_ON(PageWriteback(page));	/* Writeback must be complete */
+
 	/*
 	 * A reference is expected if PagePrivate set when move mapping,
 	 * however F2FS breaks this for maintaining dirty page counts when
 	 * truncating pages. So here adjusting the 'extra_count' make it work.
 	 */
-	extra_count = (atomic_written ? 1 : 0) - page_has_private(page);
 	rc = migrate_page_move_mapping(mapping, newpage,
-				page, NULL, mode, extra_count);
-	if (rc != MIGRATEPAGE_SUCCESS) {
-		if (atomic_written)
-			mutex_unlock(&fi->inmem_lock);
+			page, NULL, mode, (page_has_private(page) ? -1 : 0));
+	if (rc != MIGRATEPAGE_SUCCESS)
 		return rc;
-	}
-
-	if (atomic_written) {
-		struct inmem_pages *cur;
-		list_for_each_entry(cur, &fi->inmem_pages, list)
-			if (cur->page == page) {
-				cur->page = newpage;
-				break;
-			}
-		mutex_unlock(&fi->inmem_lock);
-		put_page(page);
-		get_page(newpage);
-	}
 
 	if (PagePrivate(page))
 		SetPagePrivate(newpage);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
maintainer
#1 lore raw

Hi Jaegeuk,

Quoted 3 lines · Jaegeuk Kim

In order to avoid lock contention for atomic written pages, we'd better give EAGAIN in f2fs_migrate_page. We expect it will be released soon as transaction commits.

Hmm.. if atomic write is triggered intensively, there is little change to migrate fragmented page.

How about detecting migrate mode here, for MIGRATE_SYNC case, let it moving the page; for MIGRATE_ASYNC/MIGRATE_SYNC_LIGHT case, the migration priority is lower, we can return EAGAIN.

Thanks,

Quoted 63 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)
fs/f2fs/data.c +10 -25
index d58b81213a86..1458e3a6d630 100644
@@ -2197,41 +2197,26 @@ static sector_t f2fs_bmap(struct address_space *mapping, sector_t block)
 int f2fs_migrate_page(struct address_space *mapping,
 		struct page *newpage, struct page *page, enum migrate_mode mode)
 {
-	int rc, extra_count;
-	struct f2fs_inode_info *fi = F2FS_I(mapping->host);
-	bool atomic_written = IS_ATOMIC_WRITTEN_PAGE(page);
+	int rc;
 
-	BUG_ON(PageWriteback(page));
-
-	/* migrating an atomic written page is safe with the inmem_lock hold */
-	if (atomic_written && !mutex_trylock(&fi->inmem_lock))
+	/*
+	 * We'd better return EAGAIN for atomic pages, which will be committed
+	 * sooner or later. Don't botter transactions with inmem_lock.
+	 */
+	if (IS_ATOMIC_WRITTEN_PAGE(page))
 		return -EAGAIN;
 
+	BUG_ON(PageWriteback(page));	/* Writeback must be complete */
+
 	/*
 	 * A reference is expected if PagePrivate set when move mapping,
 	 * however F2FS breaks this for maintaining dirty page counts when
 	 * truncating pages. So here adjusting the 'extra_count' make it work.
 	 */
-	extra_count = (atomic_written ? 1 : 0) - page_has_private(page);
 	rc = migrate_page_move_mapping(mapping, newpage,
-				page, NULL, mode, extra_count);
-	if (rc != MIGRATEPAGE_SUCCESS) {
-		if (atomic_written)
-			mutex_unlock(&fi->inmem_lock);
+			page, NULL, mode, (page_has_private(page) ? -1 : 0));
+	if (rc != MIGRATEPAGE_SUCCESS)
 		return rc;
-	}
-
-	if (atomic_written) {
-		struct inmem_pages *cur;
-		list_for_each_entry(cur, &fi->inmem_pages, list)
-			if (cur->page == page) {
-				cur->page = newpage;
-				break;
-			}
-		mutex_unlock(&fi->inmem_lock);
-		put_page(page);
-		get_page(newpage);
-	}
 
 	if (PagePrivate(page))
 		SetPagePrivate(newpage);
author maintainer
#2 lore raw

Re: [PATCH v2] f2fs: relax migratepage for atomic written page

In reply to Jaegeuk Kim

In order to avoid lock contention for atomic written pages, we'd better give EBUSY in f2fs_migrate_page when mode is asynchronous. We expect it will be released soon as transaction commits.

Signed-off-by Jaegeuk Kim <[email protected]>

---

Change log from v1:
- return -EBUSY when mode is MIGRATE_ASYNC or MIGRATE_SYNC_LIGHT

 fs/f2fs/data.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
fs/f2fs/data.c +6 -2
index 85ac3a63cce6..74c31cb203e2 100644
@@ -2206,8 +2206,12 @@ int f2fs_migrate_page(struct address_space *mapping,
 	BUG_ON(PageWriteback(page));
 
 	/* migrating an atomic written page is safe with the inmem_lock hold */
-	if (atomic_written && !mutex_trylock(&fi->inmem_lock))
-		return -EAGAIN;
+	if (atomic_written) {
+		if (mode != MIGRATE_SYNC)
+			return -EBUSY;
+		if (!mutex_trylock(&fi->inmem_lock))
+			return -EAGAIN;
+	}
 
 	/*
 	 * A reference is expected if PagePrivate set when move mapping,
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
maintainer Reviewed-by
#3 lore raw

Re: [f2fs-dev] [PATCH v2] f2fs: relax migratepage for atomic written page

Quoted 5 lines · Jaegeuk Kim

In order to avoid lock contention for atomic written pages, we'd better give EBUSY in f2fs_migrate_page when mode is asynchronous. We expect it will be released soon as transaction commits.

Signed-off-by Jaegeuk Kim <[email protected]>
Reviewed-by Chao Yu <[email protected]>
Quoted 28 lines

---

Change log from v1:
- return -EBUSY when mode is MIGRATE_ASYNC or MIGRATE_SYNC_LIGHT

 fs/f2fs/data.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
fs/f2fs/data.c +6 -2
index 85ac3a63cce6..74c31cb203e2 100644
@@ -2206,8 +2206,12 @@ int f2fs_migrate_page(struct address_space *mapping,
 	BUG_ON(PageWriteback(page));
 
 	/* migrating an atomic written page is safe with the inmem_lock hold */
-	if (atomic_written && !mutex_trylock(&fi->inmem_lock))
-		return -EAGAIN;
+	if (atomic_written) {
+		if (mode != MIGRATE_SYNC)
+			return -EBUSY;
+		if (!mutex_trylock(&fi->inmem_lock))
+			return -EAGAIN;
+	}
 
 	/*
 	 * A reference is expected if PagePrivate set when move mapping,