Re: [PATCH v17 07/10] fs/ntfs3: Add NTFS journal

On Thu, Dec 31, 2020 at 06:23:58PM +0300, Konstantin Komarov wrote:

+static int read_log_page(struct ntfs_log *log, u32 vbo,
+			 struct RECORD_PAGE_HDR **buffer, bool allow_errors,
+			 bool ignore_usa_error, bool *usa_error)

Allow_errors does nothing. I also think that no need for
ignore_usa_error. We can just check usa_error if we need
it. We just never raise return error for usa_error. And
then caller can decide if want's to use it. 

+{
+	int err = 0;
+	u32 page_idx = vbo >> log->page_bits;
+	u32 page_off = vbo & log->page_mask;
+	u32 bytes = log->page_size - page_off;
+	void *to_free = NULL;
+	u32 page_vbo = page_idx << log->page_bits;
+	struct RECORD_PAGE_HDR *page_buf;
+	struct ntfs_inode *ni = log->ni;
+	bool bBAAD;
+
+	if (vbo >= log->l_size)
+		return -EINVAL;
+
+	if (!*buffer) {
+		to_free = ntfs_alloc(bytes, 0);
+		if (!to_free)
+			return -ENOMEM;
+		*buffer = to_free;
+	}
+
+	page_buf = page_off ? log->one_page_buf : *buffer;
+
+	err = ntfs_read_run_nb(ni->mi.sbi, &ni->file.run, page_vbo, page_buf,
+			       log->page_size, NULL);
+	if (err)
+		goto out;
+
+	if (page_buf->rhdr.sign != NTFS_FFFF_SIGNATURE)
+		ntfs_fix_post_read(&page_buf->rhdr, PAGE_SIZE, false);
+
+	if (page_buf != *buffer)
+		memcpy(*buffer, Add2Ptr(page_buf, page_off), bytes);
+
+	bBAAD = page_buf->rhdr.sign == NTFS_BAAD_SIGNATURE;
+
+	/* Check that the update sequence array for this page is valid */
+	if (bBAAD) {
+		/* If we don't allow errors, raise an error status */
+		if (!ignore_usa_error) {
+			err = -EINVAL;
+			goto out;
+		}
+	}
+
+	if (usa_error)
+		*usa_error = bBAAD;
+

So here we can just
	delete if(bBAAD)
and use
	if (usa_error)
		*usa_error = page_buf->rhdr.sign == NTFS_BAAD_SIGNATURE;

+out:
+	if (err && to_free) {
+		ntfs_free(to_free);
+		*buffer = NULL;
+	}
+
+	return err;
+}

+/*
+ * last_log_lsn
+ *
+ * This routine walks through the log pages for a file, searching for the
+ * last log page written to the file
+ */
+static int last_log_lsn(struct ntfs_log *log)
+{

+	struct RECORD_PAGE_HDR *first_tail = NULL;
+	struct RECORD_PAGE_HDR *second_tail = NULL;

+next_tail:
+	/* Read second tail page (at pos 3/0x12000) */
+	if (read_log_page(log, second_off, &second_tail, true, true,
+			  &usa_error) ||
+	    usa_error || second_tail->rhdr.sign != NTFS_RCRD_SIGNATURE) {
+		ntfs_free(second_tail);
+		second_tail = NULL;
+		second_file_off = 0;
+		lsn2 = 0;
+	} else {
+		second_file_off = hdr_file_off(log, second_tail);
+		lsn2 = le64_to_cpu(second_tail->record_hdr.last_end_lsn);
+	}

What will happend if we get -ENOMEM from read_log_page(). Log page
might still be valid we will just ignore it. This doesn't sound 
right. 

This same thing happens many place with read_log_page().

+
+	/* Read first tail page (at pos 2/0x2000 ) */
+	if (read_log_page(log, final_off, &first_tail, true, true,
+			  &usa_error) ||
+	    usa_error || first_tail->rhdr.sign != NTFS_RCRD_SIGNATURE) {
+		ntfs_free(first_tail);
+		first_tail = NULL;
+		first_file_off = 0;
+		lsn1 = 0;
+	} else {
+		first_file_off = hdr_file_off(log, first_tail);
+		lsn1 = le64_to_cpu(first_tail->record_hdr.last_end_lsn);
+	}

+	if (first_tail && second_tail) {
+		if (best_lsn1 > best_lsn2) {
+			best_lsn = best_lsn1;
+			best_page = first_tail;
+			this_off = first_file_off;
+		} else {
+			best_lsn = best_lsn2;
+			best_page = second_tail;
+			this_off = second_file_off;
+		}
+	} else if (first_tail) {
+		best_lsn = best_lsn1;
+		best_page = first_tail;
+		this_off = first_file_off;
+	} else if (second_tail) {
+		best_lsn = best_lsn2;
+		best_page = second_tail;
+		this_off = second_file_off;
+	} else {
+		goto free_and_tail_read;

Can't we just use straight tail_read here? 

+	}
+
+	best_page_pos = le16_to_cpu(best_page->page_pos);

+	} else {
+free_and_tail_read:
+		ntfs_free(first_tail);
+		ntfs_free(second_tail);
+		goto tail_read;
+	}
+
+	ntfs_free(first_tail_prev);
+	first_tail_prev = first_tail;
+	final_off_prev = first_file_off;
+	first_tail = NULL;
+
+	ntfs_free(second_tail_prev);
+	second_tail_prev = second_tail;
+	second_off_prev = second_file_off;
+	second_tail = NULL;
+
+	final_off += log->page_size;
+	second_off += log->page_size;
+
+	if (tails < 0x10)
+		goto next_tail;
+tail_read:
+	first_tail = first_tail_prev;
+	final_off = final_off_prev;

+int log_replay(struct ntfs_inode *ni)
+{

+	/* Now we need to walk through looking for the last lsn */
+	err = last_log_lsn(log);
+	if (err == -EROFS)
+		goto out;
+

No need for this if below is whole err check.

+	if (err)
+		goto out;