Commit 6c5de280 authored by Linus Torvalds's avatar Linus Torvalds

Merge branch 'for-linus' of git://oss.sgi.com/xfs/xfs

* 'for-linus' of git://oss.sgi.com/xfs/xfs:
  xfs: improve xfs_isilocked
  xfs: skip writeback from reclaim context
  xfs: remove done roadmap item from xfs-delayed-logging-design.txt
  xfs: fix race in inode cluster freeing failing to stale inodes
  xfs: fix access to upper inodes without inode64
  xfs: fix might_sleep() warning when initialising per-ag tree
  fs/xfs/quota: Add missing mutex_unlock
  xfs: remove duplicated #include
  xfs: convert more trace events to DEFINE_EVENT
  xfs: xfs_trace.c: remove duplicated #include
  xfs: Check new inode size is OK before preallocating
  xfs: clean up xlog_align
  xfs: cleanup log reservation calculactions
  xfs: be more explicit if RT mount fails due to config
  xfs: replace E2BIG with EFBIG where appropriate
parents ed7dc1df 1bf7dbfd
......@@ -794,11 +794,6 @@ designed.
Roadmap:
2.6.35 Inclusion in mainline as an experimental mount option
=> approximately 2-3 months to merge window
=> needs to be in xfs-dev tree in 4-6 weeks
=> code is nearing readiness for review
2.6.37 Remove experimental tag from mount option
=> should be roughly 6 months after initial merge
=> enough time to:
......
......@@ -1332,6 +1332,21 @@ xfs_vm_writepage(
trace_xfs_writepage(inode, page, 0);
/*
* Refuse to write the page out if we are called from reclaim context.
*
* This is primarily to avoid stack overflows when called from deep
* used stacks in random callers for direct reclaim, but disabling
* reclaim for kswap is a nice side-effect as kswapd causes rather
* suboptimal I/O patters, too.
*
* This should really be done by the core VM, but until that happens
* filesystems like XFS, btrfs and ext4 have to take care of this
* by themselves.
*/
if (current->flags & PF_MEMALLOC)
goto out_fail;
/*
* We need a transaction if:
* 1. There are delalloc buffers on the page
......
......@@ -585,11 +585,20 @@ xfs_vn_fallocate(
bf.l_len = len;
xfs_ilock(ip, XFS_IOLOCK_EXCL);
/* check the new inode size is valid before allocating */
if (!(mode & FALLOC_FL_KEEP_SIZE) &&
offset + len > i_size_read(inode)) {
new_size = offset + len;
error = inode_newsize_ok(inode, new_size);
if (error)
goto out_unlock;
}
error = -xfs_change_file_space(ip, XFS_IOC_RESVSP, &bf,
0, XFS_ATTR_NOLOCK);
if (!error && !(mode & FALLOC_FL_KEEP_SIZE) &&
offset + len > i_size_read(inode))
new_size = offset + len;
if (error)
goto out_unlock;
/* Change file size if needed */
if (new_size) {
......@@ -600,6 +609,7 @@ xfs_vn_fallocate(
error = -xfs_setattr(ip, &iattr, XFS_ATTR_NOLOCK);
}
out_unlock:
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
out_error:
return error;
......
......@@ -23,7 +23,6 @@
#include "xfs_ag.h"
#include "xfs_mount.h"
#include "xfs_quota.h"
#include "xfs_log.h"
#include "xfs_trans.h"
#include "xfs_bmap_btree.h"
#include "xfs_inode.h"
......
......@@ -164,10 +164,6 @@ xfs_inode_ag_iterator(
struct xfs_perag *pag;
pag = xfs_perag_get(mp, ag);
if (!pag->pag_ici_init) {
xfs_perag_put(pag);
continue;
}
error = xfs_inode_ag_walk(mp, pag, execute, flags, tag,
exclusive, &nr);
xfs_perag_put(pag);
......@@ -867,12 +863,7 @@ xfs_reclaim_inode_shrink(
down_read(&xfs_mount_list_lock);
list_for_each_entry(mp, &xfs_mount_list, m_mplist) {
for (ag = 0; ag < mp->m_sb.sb_agcount; ag++) {
pag = xfs_perag_get(mp, ag);
if (!pag->pag_ici_init) {
xfs_perag_put(pag);
continue;
}
reclaimable += pag->pag_ici_reclaimable;
xfs_perag_put(pag);
}
......
......@@ -50,7 +50,6 @@
#include "quota/xfs_dquot_item.h"
#include "quota/xfs_dquot.h"
#include "xfs_log_recover.h"
#include "xfs_buf_item.h"
#include "xfs_inode_item.h"
/*
......
This diff is collapsed.
......@@ -249,9 +249,11 @@ xfs_qm_hold_quotafs_ref(
if (!xfs_Gqm) {
xfs_Gqm = xfs_Gqm_init();
if (!xfs_Gqm)
if (!xfs_Gqm) {
mutex_unlock(&xfs_Gqm_lock);
return ENOMEM;
}
}
/*
* We can keep a list of all filesystems with quotas mounted for
......
......@@ -227,7 +227,6 @@ typedef struct xfs_perag {
atomic_t pagf_fstrms; /* # of filestreams active in this AG */
int pag_ici_init; /* incore inode cache initialised */
rwlock_t pag_ici_lock; /* incore inode lock */
struct radix_tree_root pag_ici_root; /* incore inode cache root */
int pag_ici_reclaimable; /* reclaimable inodes */
......
......@@ -382,9 +382,6 @@ xfs_iget(
/* get the perag structure and ensure that it's inode capable */
pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ino));
if (!pag->pagi_inodeok)
return EINVAL;
ASSERT(pag->pag_ici_init);
agino = XFS_INO_TO_AGINO(mp, ino);
again:
......@@ -744,30 +741,24 @@ xfs_ilock_demote(
}
#ifdef DEBUG
/*
* Debug-only routine, without additional rw_semaphore APIs, we can
* now only answer requests regarding whether we hold the lock for write
* (reader state is outside our visibility, we only track writer state).
*
* Note: this means !xfs_isilocked would give false positives, so don't do that.
*/
int
xfs_isilocked(
xfs_inode_t *ip,
uint lock_flags)
{
if ((lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) ==
XFS_ILOCK_EXCL) {
if (!ip->i_lock.mr_writer)
return 0;
if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
if (!(lock_flags & XFS_ILOCK_SHARED))
return !!ip->i_lock.mr_writer;
return rwsem_is_locked(&ip->i_lock.mr_lock);
}
if ((lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) ==
XFS_IOLOCK_EXCL) {
if (!ip->i_iolock.mr_writer)
return 0;
if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
if (!(lock_flags & XFS_IOLOCK_SHARED))
return !!ip->i_iolock.mr_writer;
return rwsem_is_locked(&ip->i_iolock.mr_lock);
}
return 1;
ASSERT(0);
return 0;
}
#endif
......@@ -1940,10 +1940,10 @@ xfs_ifree_cluster(
int blks_per_cluster;
int nbufs;
int ninodes;
int i, j, found, pre_flushed;
int i, j;
xfs_daddr_t blkno;
xfs_buf_t *bp;
xfs_inode_t *ip, **ip_found;
xfs_inode_t *ip;
xfs_inode_log_item_t *iip;
xfs_log_item_t *lip;
struct xfs_perag *pag;
......@@ -1960,114 +1960,97 @@ xfs_ifree_cluster(
nbufs = XFS_IALLOC_BLOCKS(mp) / blks_per_cluster;
}
ip_found = kmem_alloc(ninodes * sizeof(xfs_inode_t *), KM_NOFS);
for (j = 0; j < nbufs; j++, inum += ninodes) {
int found = 0;
blkno = XFS_AGB_TO_DADDR(mp, XFS_INO_TO_AGNO(mp, inum),
XFS_INO_TO_AGBNO(mp, inum));
/*
* We obtain and lock the backing buffer first in the process
* here, as we have to ensure that any dirty inode that we
* can't get the flush lock on is attached to the buffer.
* If we scan the in-memory inodes first, then buffer IO can
* complete before we get a lock on it, and hence we may fail
* to mark all the active inodes on the buffer stale.
*/
bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, blkno,
mp->m_bsize * blks_per_cluster,
XBF_LOCK);
/*
* Look for each inode in memory and attempt to lock it,
* we can be racing with flush and tail pushing here.
* any inode we get the locks on, add to an array of
* inode items to process later.
* Walk the inodes already attached to the buffer and mark them
* stale. These will all have the flush locks held, so an
* in-memory inode walk can't lock them.
*/
lip = XFS_BUF_FSPRIVATE(bp, xfs_log_item_t *);
while (lip) {
if (lip->li_type == XFS_LI_INODE) {
iip = (xfs_inode_log_item_t *)lip;
ASSERT(iip->ili_logged == 1);
lip->li_cb = (void(*)(xfs_buf_t*,xfs_log_item_t*)) xfs_istale_done;
xfs_trans_ail_copy_lsn(mp->m_ail,
&iip->ili_flush_lsn,
&iip->ili_item.li_lsn);
xfs_iflags_set(iip->ili_inode, XFS_ISTALE);
found++;
}
lip = lip->li_bio_list;
}
/*
* For each inode in memory attempt to add it to the inode
* buffer and set it up for being staled on buffer IO
* completion. This is safe as we've locked out tail pushing
* and flushing by locking the buffer.
*
* The get the buffer lock, we could beat a flush
* or tail pushing thread to the lock here, in which
* case they will go looking for the inode buffer
* and fail, we need some other form of interlock
* here.
* We have already marked every inode that was part of a
* transaction stale above, which means there is no point in
* even trying to lock them.
*/
found = 0;
for (i = 0; i < ninodes; i++) {
read_lock(&pag->pag_ici_lock);
ip = radix_tree_lookup(&pag->pag_ici_root,
XFS_INO_TO_AGINO(mp, (inum + i)));
/* Inode not in memory or we found it already,
* nothing to do
*/
/* Inode not in memory or stale, nothing to do */
if (!ip || xfs_iflags_test(ip, XFS_ISTALE)) {
read_unlock(&pag->pag_ici_lock);
continue;
}
if (xfs_inode_clean(ip)) {
/* don't try to lock/unlock the current inode */
if (ip != free_ip &&
!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
read_unlock(&pag->pag_ici_lock);
continue;
}
/* If we can get the locks then add it to the
* list, otherwise by the time we get the bp lock
* below it will already be attached to the
* inode buffer.
*/
/* This inode will already be locked - by us, lets
* keep it that way.
*/
if (ip == free_ip) {
if (xfs_iflock_nowait(ip)) {
xfs_iflags_set(ip, XFS_ISTALE);
if (xfs_inode_clean(ip)) {
xfs_ifunlock(ip);
} else {
ip_found[found++] = ip;
}
}
read_unlock(&pag->pag_ici_lock);
if (!xfs_iflock_nowait(ip)) {
if (ip != free_ip)
xfs_iunlock(ip, XFS_ILOCK_EXCL);
continue;
}
if (xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
if (xfs_iflock_nowait(ip)) {
xfs_iflags_set(ip, XFS_ISTALE);
if (xfs_inode_clean(ip)) {
ASSERT(ip != free_ip);
xfs_ifunlock(ip);
xfs_iunlock(ip, XFS_ILOCK_EXCL);
} else {
ip_found[found++] = ip;
}
} else {
xfs_iunlock(ip, XFS_ILOCK_EXCL);
}
}
read_unlock(&pag->pag_ici_lock);
}
bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, blkno,
mp->m_bsize * blks_per_cluster,
XBF_LOCK);
pre_flushed = 0;
lip = XFS_BUF_FSPRIVATE(bp, xfs_log_item_t *);
while (lip) {
if (lip->li_type == XFS_LI_INODE) {
iip = (xfs_inode_log_item_t *)lip;
ASSERT(iip->ili_logged == 1);
lip->li_cb = (void(*)(xfs_buf_t*,xfs_log_item_t*)) xfs_istale_done;
xfs_trans_ail_copy_lsn(mp->m_ail,
&iip->ili_flush_lsn,
&iip->ili_item.li_lsn);
xfs_iflags_set(iip->ili_inode, XFS_ISTALE);
pre_flushed++;
}
lip = lip->li_bio_list;
continue;
}
for (i = 0; i < found; i++) {
ip = ip_found[i];
iip = ip->i_itemp;
if (!iip) {
/* inode with unlogged changes only */
ASSERT(ip != free_ip);
ip->i_update_core = 0;
xfs_ifunlock(ip);
xfs_iunlock(ip, XFS_ILOCK_EXCL);
continue;
}
found++;
iip->ili_last_fields = iip->ili_format.ilf_fields;
iip->ili_format.ilf_fields = 0;
......@@ -2078,17 +2061,16 @@ xfs_ifree_cluster(
xfs_buf_attach_iodone(bp,
(void(*)(xfs_buf_t*,xfs_log_item_t*))
xfs_istale_done, (xfs_log_item_t *)iip);
if (ip != free_ip) {
if (ip != free_ip)
xfs_iunlock(ip, XFS_ILOCK_EXCL);
}
}
if (found || pre_flushed)
if (found)
xfs_trans_stale_inode_buf(tp, bp);
xfs_trans_binval(tp, bp);
}
kmem_free(ip_found);
xfs_perag_put(pag);
}
......@@ -2649,8 +2631,6 @@ xfs_iflush_cluster(
int i;
pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
ASSERT(pag->pagi_inodeok);
ASSERT(pag->pag_ici_init);
inodes_per_cluster = XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog;
ilist_size = inodes_per_cluster * sizeof(xfs_inode_t *);
......
......@@ -132,15 +132,10 @@ xlog_align(
int nbblks,
xfs_buf_t *bp)
{
xfs_daddr_t offset;
xfs_caddr_t ptr;
offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));
xfs_daddr_t offset = blk_no & ((xfs_daddr_t)log->l_sectBBsize - 1);
return ptr;
ASSERT(BBTOB(offset + nbblks) <= XFS_BUF_SIZE(bp));
return XFS_BUF_PTR(bp) + BBTOB(offset);
}
......
......@@ -268,10 +268,10 @@ xfs_sb_validate_fsb_count(
#if XFS_BIG_BLKNOS /* Limited by ULONG_MAX of page cache index */
if (nblocks >> (PAGE_CACHE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
return E2BIG;
return EFBIG;
#else /* Limited by UINT_MAX of sectors */
if (nblocks << (sbp->sb_blocklog - BBSHIFT) > UINT_MAX)
return E2BIG;
return EFBIG;
#endif
return 0;
}
......@@ -393,7 +393,7 @@ xfs_mount_validate_sb(
xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) {
xfs_fs_mount_cmn_err(flags,
"file system too large to be mounted on this system.");
return XFS_ERROR(E2BIG);
return XFS_ERROR(EFBIG);
}
if (unlikely(sbp->sb_inprogress)) {
......@@ -413,17 +413,6 @@ xfs_mount_validate_sb(
return 0;
}
STATIC void
xfs_initialize_perag_icache(
xfs_perag_t *pag)
{
if (!pag->pag_ici_init) {
rwlock_init(&pag->pag_ici_lock);
INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
pag->pag_ici_init = 1;
}
}
int
xfs_initialize_perag(
xfs_mount_t *mp,
......@@ -436,13 +425,8 @@ xfs_initialize_perag(
xfs_agino_t agino;
xfs_ino_t ino;
xfs_sb_t *sbp = &mp->m_sb;
xfs_ino_t max_inum = XFS_MAXINUMBER_32;
int error = -ENOMEM;
/* Check to see if the filesystem can overflow 32 bit inodes */
agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
/*
* Walk the current per-ag tree so we don't try to initialise AGs
* that already exist (growfs case). Allocate and insert all the
......@@ -456,11 +440,18 @@ xfs_initialize_perag(
}
if (!first_initialised)
first_initialised = index;
pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
if (!pag)
goto out_unwind;
pag->pag_agno = index;
pag->pag_mount = mp;
rwlock_init(&pag->pag_ici_lock);
INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
if (radix_tree_preload(GFP_NOFS))
goto out_unwind;
spin_lock(&mp->m_perag_lock);
if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
BUG();
......@@ -469,25 +460,26 @@ xfs_initialize_perag(
error = -EEXIST;
goto out_unwind;
}
pag->pag_agno = index;
pag->pag_mount = mp;
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
}
/* Clear the mount flag if no inode can overflow 32 bits
* on this filesystem, or if specifically requested..
/*
* If we mount with the inode64 option, or no inode overflows
* the legacy 32-bit address space clear the inode32 option.
*/
if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > max_inum) {
agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
mp->m_flags |= XFS_MOUNT_32BITINODES;
} else {
else
mp->m_flags &= ~XFS_MOUNT_32BITINODES;
}
/* If we can overflow then setup the ag headers accordingly */
if (mp->m_flags & XFS_MOUNT_32BITINODES) {
/* Calculate how much should be reserved for inodes to
* meet the max inode percentage.
/*
* Calculate how much should be reserved for inodes to meet
* the max inode percentage.
*/
if (mp->m_maxicount) {
__uint64_t icount;
......@@ -500,30 +492,28 @@ xfs_initialize_perag(
} else {
max_metadata = agcount;
}
for (index = 0; index < agcount; index++) {
ino = XFS_AGINO_TO_INO(mp, index, agino);
if (ino > max_inum) {
if (ino > XFS_MAXINUMBER_32) {
index++;
break;
}
/* This ag is preferred for inodes */
pag = xfs_perag_get(mp, index);
pag->pagi_inodeok = 1;
if (index < max_metadata)
pag->pagf_metadata = 1;
xfs_initialize_perag_icache(pag);
xfs_perag_put(pag);
}
} else {
/* Setup default behavior for smaller filesystems */
for (index = 0; index < agcount; index++) {
pag = xfs_perag_get(mp, index);
pag->pagi_inodeok = 1;
xfs_initialize_perag_icache(pag);
xfs_perag_put(pag);
}
}
if (maxagi)
*maxagi = index;
return 0;
......@@ -1009,7 +999,7 @@ xfs_check_sizes(xfs_mount_t *mp)
d = (xfs_daddr_t)XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
if (XFS_BB_TO_FSB(mp, d) != mp->m_sb.sb_dblocks) {
cmn_err(CE_WARN, "XFS: size check 1 failed");
return XFS_ERROR(E2BIG);
return XFS_ERROR(EFBIG);
}
error = xfs_read_buf(mp, mp->m_ddev_targp,
d - XFS_FSS_TO_BB(mp, 1),
......@@ -1019,7 +1009,7 @@ xfs_check_sizes(xfs_mount_t *mp)
} else {
cmn_err(CE_WARN, "XFS: size check 2 failed");
if (error == ENOSPC)
error = XFS_ERROR(E2BIG);
error = XFS_ERROR(EFBIG);
return error;
}
......@@ -1027,7 +1017,7 @@ xfs_check_sizes(xfs_mount_t *mp)
d = (xfs_daddr_t)XFS_FSB_TO_BB(mp, mp->m_sb.sb_logblocks);
if (XFS_BB_TO_FSB(mp, d) != mp->m_sb.sb_logblocks) {
cmn_err(CE_WARN, "XFS: size check 3 failed");
return XFS_ERROR(E2BIG);
return XFS_ERROR(EFBIG);
}
error = xfs_read_buf(mp, mp->m_logdev_targp,
d - XFS_FSB_TO_BB(mp, 1),
......@@ -1037,7 +1027,7 @@ xfs_check_sizes(xfs_mount_t *mp)
} else {
cmn_err(CE_WARN, "XFS: size check 3 failed");
if (error == ENOSPC)
error = XFS_ERROR(E2BIG);
error = XFS_ERROR(EFBIG);
return error;
}
}
......@@ -1254,7 +1244,7 @@ xfs_mountfs(
* Allocate and initialize the per-ag data.
*/
spin_lock_init(&mp->m_perag_lock);
INIT_RADIX_TREE(&mp->m_perag_tree, GFP_NOFS);
INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC);
error = xfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
if (error) {
cmn_err(CE_WARN, "XFS: Failed per-ag init: %d", error);
......
......@@ -2247,7 +2247,7 @@ xfs_rtmount_init(
cmn_err(CE_WARN, "XFS: realtime mount -- %llu != %llu",
(unsigned long long) XFS_BB_TO_FSB(mp, d),
(unsigned long long) mp->m_sb.sb_rblocks);
return XFS_ERROR(E2BIG);
return XFS_ERROR(EFBIG);
}
error = xfs_read_buf(mp, mp->m_rtdev_targp,
d - XFS_FSB_TO_BB(mp, 1),
......@@ -2256,7 +2256,7 @@ xfs_rtmount_init(
cmn_err(CE_WARN,
"XFS: realtime mount -- xfs_read_buf failed, returned %d", error);
if (error == ENOSPC)
return XFS_ERROR(E2BIG);
return XFS_ERROR(EFBIG);
return error;
}
xfs_buf_relse(bp);
......
......@@ -147,7 +147,16 @@ xfs_growfs_rt(
# define xfs_rtfree_extent(t,b,l) (ENOSYS)
# define xfs_rtpick_extent(m,t,l,rb) (ENOSYS)
# define xfs_growfs_rt(mp,in) (ENOSYS)
# define xfs_rtmount_init(m) (((mp)->m_sb.sb_rblocks == 0)? 0 : (ENOSYS))
static inline int /* error */
xfs_rtmount_init(
xfs_mount_t *mp) /* file system mount structure */
{
if (mp->m_sb.sb_rblocks == 0)
return 0;
cmn_err(CE_WARN, "XFS: Not built with CONFIG_XFS_RT");
return ENOSYS;
}
# define xfs_rtmount_inodes(m) (((mp)->m_sb.sb_rblocks == 0)? 0 : (ENOSYS))
# define xfs_rtunmount_inodes(m)
#endif /* CONFIG_XFS_RT */
......
This diff is collapsed.
This diff is collapsed.
......@@ -267,7 +267,7 @@ xfs_setattr(
if (code) {
ASSERT(tp == NULL);
lock_flags &= ~XFS_ILOCK_EXCL;
ASSERT(lock_flags == XFS_IOLOCK_EXCL);
ASSERT(lock_flags == XFS_IOLOCK_EXCL || !need_iolock);
goto error_return;
}
tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_SIZE);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment