linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kari Argillander <kari.argillander@gmail.com>
To: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
	linux-kernel@vger.kernel.org, pali@kernel.org, dsterba@suse.cz,
	aaptel@suse.com, willy@infradead.org, rdunlap@infradead.org,
	joe@perches.com, mark@harmstone.com, nborisov@suse.com,
	linux-ntfs-dev@lists.sourceforge.net, anton@tuxera.com,
	dan.carpenter@oracle.com, hch@lst.de, ebiggers@kernel.org,
	andy.lavr@gmail.com
Subject: Re: [PATCH v17 07/10] fs/ntfs3: Add NTFS journal
Date: Mon, 4 Jan 2021 00:47:35 +0200	[thread overview]
Message-ID: <20210103224735.gtirbmcpkdsietrl@kari-VirtualBox> (raw)
In-Reply-To: <20201231152401.3162425-8-almaz.alexandrovich@paragon-software.com>

On Thu, Dec 31, 2020 at 06:23:58PM +0300, Konstantin Komarov wrote:
> This adds NTFS journal
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/fslog.c | 5220 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 5220 insertions(+)
>  create mode 100644 fs/ntfs3/fslog.c
> 
> diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c

> +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;
 

  reply	other threads:[~2021-01-03 22:48 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-31 15:23 [PATCH v17 00/10] NTFS read-write driver GPL implementation by Paragon Software Konstantin Komarov
2020-12-31 15:23 ` [PATCH v17 01/10] fs/ntfs3: Add headers and misc files Konstantin Komarov
2021-01-03 23:17   ` Kari Argillander
2021-01-19 10:43     ` Dan Carpenter
2021-01-19 20:34       ` Kari Argillander
2020-12-31 15:23 ` [PATCH v17 02/10] fs/ntfs3: Add initialization of super block Konstantin Komarov
2021-01-03 19:50   ` Kari Argillander
2021-01-18  9:35     ` Konstantin Komarov
2021-01-18 14:09       ` Kari Argillander
2021-01-19  4:03   ` Kari Argillander
2021-01-20  7:20     ` Kari Argillander
2020-12-31 15:23 ` [PATCH v17 03/10] fs/ntfs3: Add bitmap Konstantin Komarov
2020-12-31 15:23 ` [PATCH v17 04/10] fs/ntfs3: Add file operations and implementation Konstantin Komarov
2021-01-03 21:57   ` Kari Argillander
2021-01-03 22:04     ` Matthew Wilcox
2021-01-18 10:00     ` Konstantin Komarov
2021-01-18 14:24       ` Kari Argillander
2021-01-04  2:18   ` Matthew Wilcox
2020-12-31 15:23 ` [PATCH v17 05/10] fs/ntfs3: Add attrib operations Konstantin Komarov
2021-01-04  0:25   ` Kari Argillander
2021-01-18 12:01     ` Konstantin Komarov
2020-12-31 15:23 ` [PATCH v17 06/10] fs/ntfs3: Add compression Konstantin Komarov
2020-12-31 15:23 ` [PATCH v17 07/10] fs/ntfs3: Add NTFS journal Konstantin Komarov
2021-01-03 22:47   ` Kari Argillander [this message]
2020-12-31 15:23 ` [PATCH v17 08/10] fs/ntfs3: Add Kconfig, Makefile and doc Konstantin Komarov
2021-01-03 22:07   ` Kari Argillander
2021-01-18 11:43     ` Konstantin Komarov
2021-01-18 13:42       ` Mark Harmstone
2020-12-31 15:24 ` [PATCH v17 09/10] fs/ntfs3: Add NTFS3 in fs/Kconfig and fs/Makefile Konstantin Komarov
2020-12-31 15:24 ` [PATCH v17 10/10] fs/ntfs3: Add MAINTAINERS Konstantin Komarov
2021-01-03 20:01   ` Kari Argillander

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210103224735.gtirbmcpkdsietrl@kari-VirtualBox \
    --to=kari.argillander@gmail.com \
    --cc=aaptel@suse.com \
    --cc=almaz.alexandrovich@paragon-software.com \
    --cc=andy.lavr@gmail.com \
    --cc=anton@tuxera.com \
    --cc=dan.carpenter@oracle.com \
    --cc=dsterba@suse.cz \
    --cc=ebiggers@kernel.org \
    --cc=hch@lst.de \
    --cc=joe@perches.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-ntfs-dev@lists.sourceforge.net \
    --cc=mark@harmstone.com \
    --cc=nborisov@suse.com \
    --cc=pali@kernel.org \
    --cc=rdunlap@infradead.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).